Fix memory leaks & Improve README (#24)

The `Test` Gh workflow is enhanced by also running the whole test suite
with the `LEAK_CHECK` environment variable.
While testing this functionality, it became apparent that we had memory
leaks on `CBLCollection`, `CBLScope` & `CBLQueryIndex` objects.

The functions `retain` and `wrap` were not clear enough on what they
did, especially with the doc comments that were inverted. So the changes
are:

1. Rename `retain` to `reference`
2. Rename `wrap` to `take_ownership`
3. Fix the doc comments on all functions
4. Fix the cases where retain and wrap were misused for `CBLCollection`,
`CBLScope` & `CBLQueryIndex`
5. Implement `Drop` for `QueryIndex`

The tests are now passing with the `LEAK_CHECK` environment variable.

Additional changes:

- The README is improved to document how to test
- The README is improved to make editions (`community` & `enterprise`)
more understandable
- The file `src/main.rs` is deleted as this is a library

Author: Nao-ris

.github/workflows/test.yml

@@ -24,5 +24,7 @@ jobs:
       - uses: actions/checkout@v3
       - name: Run tests
         run: cargo test --features=${{ matrix.version }} --verbose
+      - name: Run tests with Couchbase Lite C leak check
+        run: LEAK_CHECK=y cargo test --features=${{ matrix.version }} --verbose -- --test-threads 1
       - name: Run tests (with address sanitizer)
         run: LSAN_OPTIONS=suppressions=san.supp RUSTFLAGS="-Zsanitizer=address" cargo +nightly test --features=${{ matrix.version }} --verbose

README.md

@@ -20,22 +20,25 @@ Installation instructions are [here][BINDGEN_INSTALL].
 
 ### 2. Build!
 
-You can use Couchbase Lite C community or entreprise editions:
+There two different editions of Couchbase Lite C: community & enterprise.
+You can find the differences [here][CBL_EDITIONS_DIFF].
+
+When building or declaring this repository as a dependency, you need to specify the edition through a cargo feature:
 
 ```shell
-$ cargo build --features=enterprise
+$ cargo build --features=community
 ```
 
 ```shell
-$ cargo build --features=community
+$ cargo build --features=enterprise
 ```
 
 ## Maintaining
 
 ### Couchbase Lite For C
 
 The Couchbase Lite For C shared library and headers ([Git repo][CBL_C]) are already embedded in this repo.
-They are present in the directory `libcblite`.
+They are present in two directories, one for each edition: `libcblite_community` & `libcblite_enterprise`.
 
 ### Upgrade Couchbase Lite C
 
@@ -54,24 +57,46 @@ $ brew install wget
 $ brew install bash
 ```
 
-After that, fix the compilation & tests and you can create a pull request.
+If the script was successful:
+- Change the link `CBL_API_REFERENCE` in this README
+- Change the version in the test `couchbase_lite_c_version_test`
+- Update the version in `Cargo.toml`
+- Fix the compilation in both editions
+- Fix the tests in both editions
+- Create pull request
 
 New C features should also be added to the Rust API at some point.
 
 ### Test
 
-**The unit tests must be run single-threaded.** This is because each test case checks for leaks by
-counting the number of extant Couchbase Lite objects before and after it runs, and failing if the
-number increases. That works only if a single test runs at a time.
+Tests can be found in the `tests` subdirectory.
+Test are run in the GitHub wrokflow `Test`. You can find the commands used there.
+
+There are three variations:
+
+### Nominal run
 
 ```shell
-$ LEAK_CHECK=y cargo test -- --test-threads 1
+$ cargo test --features=enterprise
 ```
 
-### Sanitizer
+### Run with Couchbase Lite C leak check
+
+Couchbase Lite C allows checking if instances of their objects are still alive through the functions `CBL_InstanceCount` & `CBL_DumpInstances`.
+If the `LEAK_CHECK` environment variable is set, we check that the number of instances at the end of each test is 0.
+
+If this step fails in one of your pull requests, you should look into the `take_ownership`/`reference` logic on CBL pointers in the constructor of the Rust structs:
+- `take_ownership` takes ownership of the pointer, it will not increase the ref count of the `ref` CBL pointer so releasing it (in a `drop` for example) will free the pointer
+- `reference` just references the pointer, it will increase the ref count of CBL pointers so releasing it will not free the pointer
 
 ```shell
-$ LSAN_OPTIONS=suppressions=san.supp RUSTFLAGS="-Zsanitizer=address" cargo +nightly test 
+$ LEAK_CHECK=y cargo test --features=enterprise -- --test-threads 1
+```
+
+### Run with address sanitizer
+
+```shell
+$ LSAN_OPTIONS=suppressions=san.supp RUSTFLAGS="-Zsanitizer=address" cargo +nightly test  --features=enterprise
 ```
 
 ## Learning
@@ -96,6 +121,8 @@ $ LSAN_OPTIONS=suppressions=san.supp RUSTFLAGS="-Zsanitizer=address" cargo +nigh
 
 [CBL_API_REFERENCE]: https://docs.couchbase.com/mobile/3.2.1/couchbase-lite-c/C/html/modules.html
 
+[CBL_EDITIONS_DIFF]: https://www.couchbase.com/products/editions/
+
 [FLEECE]: https://github.com/couchbaselabs/fleece/wiki/Using-Fleece
 
 [BINDGEN]: https://rust-lang.github.io/rust-bindgen/

src/collection.rs

@@ -56,18 +56,23 @@ impl Collection {
 
     //////// CONSTRUCTORS:
 
-    /// Takes ownership of the object and increase it's reference counter.
-    pub(crate) fn retain(cbl_ref: *mut CBLCollection) -> Self {
+    /// Increase the reference counter of the CBL ref, so dropping the instance will NOT free the ref.
+    pub(crate) fn reference(cbl_ref: *mut CBLCollection) -> Self {
         Self {
             cbl_ref: unsafe { retain(cbl_ref) },
         }
     }
 
+    /// Takes ownership of the CBL ref, the reference counter is not increased so dropping the instance will free the ref.
+    pub(crate) const fn take_ownership(cbl_ref: *mut CBLCollection) -> Self {
+        Self { cbl_ref }
+    }
+
     ////////
 
     /// Returns the scope of the collection.
     pub fn scope(&self) -> Scope {
-        unsafe { Scope::retain(CBLCollection_Scope(self.get_ref())) }
+        unsafe { Scope::take_ownership(CBLCollection_Scope(self.get_ref())) }
     }
 
     /// Returns the collection name.
@@ -90,7 +95,7 @@ impl Collection {
 
     /// Returns the collection's database.
     pub fn database(&self) -> Database {
-        unsafe { Database::wrap(CBLCollection_Database(self.get_ref())) }
+        unsafe { Database::reference(CBLCollection_Database(self.get_ref())) }
     }
 
     /// Returns the number of documents in the collection.
@@ -136,7 +141,7 @@ impl Drop for Collection {
 
 impl Clone for Collection {
     fn clone(&self) -> Self {
-        Self::retain(self.get_ref())
+        Self::reference(self.get_ref())
     }
 }
 
@@ -150,7 +155,7 @@ unsafe extern "C" fn c_collection_change_listener(
 ) {
     let callback = context as *const CollectionChangeListener;
     if let Some(change) = change.as_ref() {
-        let collection = Collection::retain(change.collection as *mut CBLCollection);
+        let collection = Collection::reference(change.collection as *mut CBLCollection);
         let doc_ids = std::slice::from_raw_parts(change.docIDs, change.numDocs as usize)
             .iter()
             .filter_map(|doc_id| doc_id.to_string())

src/database.rs

@@ -140,7 +140,7 @@ unsafe extern "C" fn c_database_change_listener(
     c_doc_ids: *mut FLString,
 ) {
     let callback = context as *const DatabaseChangeListener;
-    let database = Database::retain(db as *mut CBLDatabase);
+    let database = Database::reference(db as *mut CBLDatabase);
 
     let doc_ids = std::slice::from_raw_parts(c_doc_ids, num_docs as usize)
         .iter()
@@ -159,7 +159,7 @@ unsafe extern "C" fn c_database_buffer_notifications(
 ) {
     let callback: BufferNotifications = std::mem::transmute(context);
 
-    let database = Database::retain(db.cast::<CBLDatabase>());
+    let database = Database::reference(db.cast::<CBLDatabase>());
 
     callback(&database);
 }
@@ -180,15 +180,15 @@ impl CblRef for Database {
 impl Database {
     //////// CONSTRUCTORS:
 
-    /// Takes ownership of the object and increase it's reference counter.
-    pub(crate) fn retain(cbl_ref: *mut CBLDatabase) -> Self {
+    /// Increase the reference counter of the CBL ref, so dropping the instance will NOT free the ref.
+    pub(crate) fn reference(cbl_ref: *mut CBLDatabase) -> Self {
         Self {
             cbl_ref: unsafe { retain(cbl_ref) },
         }
     }
 
-    /// References the object without taking ownership and increasing it's reference counter
-    pub(crate) const fn wrap(cbl_ref: *mut CBLDatabase) -> Self {
+    /// Takes ownership of the CBL ref, the reference counter is not increased so dropping the instance will free the ref.
+    pub(crate) const fn take_ownership(cbl_ref: *mut CBLDatabase) -> Self {
         Self { cbl_ref }
     }
 
@@ -217,7 +217,7 @@ impl Database {
         if db_ref.is_null() {
             return failure(err);
         }
-        Ok(Self::wrap(db_ref))
+        Ok(Self::take_ownership(db_ref))
     }
 
     //////// OTHER STATIC METHODS:
@@ -418,7 +418,7 @@ impl Database {
             if scope.is_null() {
                 None
             } else {
-                Some(Scope::retain(scope))
+                Some(Scope::take_ownership(scope))
             }
         })
     }
@@ -445,7 +445,7 @@ impl Database {
             if collection.is_null() {
                 None
             } else {
-                Some(Collection::retain(collection))
+                Some(Collection::take_ownership(collection))
             }
         })
     }
@@ -474,7 +474,7 @@ impl Database {
             )
         };
 
-        check_error(&error).map(|()| Collection::retain(collection))
+        check_error(&error).map(|()| Collection::take_ownership(collection))
     }
 
     /// Delete an existing collection.
@@ -499,7 +499,7 @@ impl Database {
         let mut error = CBLError::default();
         let scope = unsafe { CBLDatabase_DefaultScope(self.get_ref(), &mut error) };
 
-        check_error(&error).map(|()| Scope::retain(scope))
+        check_error(&error).map(|()| Scope::take_ownership(scope))
     }
 
     /// Returns the default collection.
@@ -511,7 +511,7 @@ impl Database {
             if collection.is_null() {
                 None
             } else {
-                Some(Collection::retain(collection))
+                Some(Collection::take_ownership(collection))
             }
         })
     }
@@ -526,7 +526,7 @@ impl Database {
         if collection.is_null() {
             Err(Error::cbl_error(CouchbaseLiteError::NotFound))
         } else {
-            Ok(Collection::retain(collection))
+            Ok(Collection::take_ownership(collection))
         }
     }
 
@@ -596,6 +596,6 @@ impl Drop for Database {
 
 impl Clone for Database {
     fn clone(&self) -> Self {
-        Self::retain(self.get_ref())
+        Self::reference(self.get_ref())
     }
 }

src/document.rs

@@ -75,8 +75,8 @@ unsafe extern "C" fn c_conflict_handler(
     let callback: ConflictHandler = std::mem::transmute(context);
 
     callback(
-        &mut Document::retain(document_being_saved),
-        &Document::retain(conflicting_document as *mut CBLDocument),
+        &mut Document::reference(document_being_saved),
+        &Document::reference(conflicting_document as *mut CBLDocument),
     )
 }
 
@@ -92,7 +92,7 @@ unsafe extern "C" fn c_database_document_change_listener(
     c_doc_id: FLString,
 ) {
     let callback = context as *const DatabaseDocumentChangeListener;
-    let database = Database::retain(db as *mut CBLDatabase);
+    let database = Database::reference(db as *mut CBLDatabase);
     (*callback)(&database, c_doc_id.to_string());
 }
 
@@ -116,7 +116,7 @@ impl Database {
                     failure(error)
                 };
             }
-            Ok(Document::wrap(doc))
+            Ok(Document::take_ownership(doc))
         }
     }
 
@@ -315,7 +315,7 @@ unsafe extern "C" fn c_collection_document_change_listener(
 ) {
     let callback = context as *const CollectionDocumentChangeListener;
     if let Some(change) = change.as_ref() {
-        let collection = Collection::retain(change.collection as *mut CBLCollection);
+        let collection = Collection::reference(change.collection as *mut CBLCollection);
         (*callback)(collection, change.docID.to_string());
     }
 }
@@ -340,7 +340,7 @@ impl Collection {
                     failure(error)
                 };
             }
-            Ok(Document::wrap(doc))
+            Ok(Document::take_ownership(doc))
         }
     }
 
@@ -515,26 +515,26 @@ impl Document {
     /// Creates a new, empty document in memory, with an automatically generated unique ID.
     /// It will not be added to a database until saved.
     pub fn new() -> Self {
-        unsafe { Self::wrap(CBLDocument_Create()) }
+        unsafe { Self::take_ownership(CBLDocument_Create()) }
     }
 
     /// Creates a new, empty document in memory, with the given ID.
     /// It will not be added to a database until saved.
     pub fn new_with_id(id: &str) -> Self {
-        unsafe { Self::wrap(CBLDocument_CreateWithID(from_str(id).get_ref())) }
+        unsafe { Self::take_ownership(CBLDocument_CreateWithID(from_str(id).get_ref())) }
     }
 
-    /// Takes ownership of the object and increase it's reference counter.
-    pub(crate) fn retain(cbl_ref: *mut CBLDocument) -> Self {
+    /// Increase the reference counter of the CBL ref, so dropping the instance will NOT free the ref.
+    pub(crate) fn reference(cbl_ref: *mut CBLDocument) -> Self {
         unsafe {
             Self {
                 cbl_ref: retain(cbl_ref),
             }
         }
     }
 
-    /// References the object without taking ownership and increasing it's reference counter
-    pub(crate) const fn wrap(cbl_ref: *mut CBLDocument) -> Self {
+    /// Takes ownership of the CBL ref, the reference counter is not increased so dropping the instance will free the ref.
+    pub(crate) const fn take_ownership(cbl_ref: *mut CBLDocument) -> Self {
         Self { cbl_ref }
     }
 
@@ -607,6 +607,6 @@ impl Drop for Document {
 
 impl Clone for Document {
     fn clone(&self) -> Self {
-        Self::retain(self.get_ref())
+        Self::reference(self.get_ref())
     }
 }

src/encryptable.rs

@@ -66,15 +66,15 @@ impl CblRef for Encryptable {
 
 impl From<*mut CBLEncryptable> for Encryptable {
     fn from(cbl_ref: *mut CBLEncryptable) -> Self {
-        Self::retain(cbl_ref)
+        Self::reference(cbl_ref)
     }
 }
 
 impl Encryptable {
     //////// CONSTRUCTORS:
 
-    /// Takes ownership of the object and increase it's reference counter.
-    pub(crate) fn retain(cbl_ref: *mut CBLEncryptable) -> Self {
+    /// Increase the reference counter of the CBL ref, so dropping the instance will NOT free the ref.
+    pub(crate) fn reference(cbl_ref: *mut CBLEncryptable) -> Self {
         Self {
             cbl_ref: unsafe { retain(cbl_ref) },
         }

src/fleece.rs

@@ -323,7 +323,7 @@ impl Value {
     pub fn get_encryptable_value(&self) -> Encryptable {
         unsafe {
             let encryptable = FLDict_GetEncryptableValue(FLValue_AsDict(self.get_ref()));
-            Encryptable::retain(encryptable as *mut CBLEncryptable)
+            Encryptable::reference(encryptable as *mut CBLEncryptable)
         }
     }
 
@@ -598,7 +598,7 @@ impl Dict {
     pub fn get_encryptable_value(&self) -> Encryptable {
         unsafe {
             let encryptable = FLDict_GetEncryptableValue(self.get_ref());
-            Encryptable::retain(encryptable as *mut CBLEncryptable)
+            Encryptable::reference(encryptable as *mut CBLEncryptable)
         }
     }