diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 1d952d6..0e6ca29 100644 --- a/.github/workflows/test.yml +++ b/.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 diff --git a/README.md b/README.md index c54013b..08c8677 100644 --- a/README.md +++ b/README.md @@ -20,14 +20,17 @@ 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 @@ -35,7 +38,7 @@ $ cargo build --features=community ### 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/ diff --git a/src/collection.rs b/src/collection.rs index 0a79daa..5c78916 100644 --- a/src/collection.rs +++ b/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()) diff --git a/src/database.rs b/src/database.rs index 169d5d3..eb63c31 100644 --- a/src/database.rs +++ b/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::()); + let database = Database::reference(db.cast::()); 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()) } } diff --git a/src/document.rs b/src/document.rs index 8a1695c..6b12e46 100644 --- a/src/document.rs +++ b/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,17 +515,17 @@ 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), @@ -533,8 +533,8 @@ impl Document { } } - /// 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()) } } diff --git a/src/encryptable.rs b/src/encryptable.rs index 40e4c5c..e5a0f17 100644 --- a/src/encryptable.rs +++ b/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) }, } diff --git a/src/fleece.rs b/src/fleece.rs index 7f7dd37..a73ba88 100644 --- a/src/fleece.rs +++ b/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) } } diff --git a/src/index.rs b/src/index.rs index 89fad98..ecbb5c9 100644 --- a/src/index.rs +++ b/src/index.rs @@ -10,7 +10,7 @@ use crate::{ slice::{from_str, from_c_str, Slice}, QueryLanguage, Array, collection::Collection, - check_error, retain, CouchbaseLiteError, + check_error, release, retain, CouchbaseLiteError, }; use std::ffi::CString; @@ -148,13 +148,19 @@ impl CblRef for QueryIndex { impl QueryIndex { //////// CONSTRUCTORS: - /// Takes ownership of the object and increase it's reference counter. - pub(crate) fn retain(cbl_ref: *mut CBLQueryIndex) -> Self { + /// Increase the reference counter of the CBL ref, so dropping the instance will NOT free the ref. + #[allow(dead_code)] + pub(crate) fn reference(cbl_ref: *mut CBLQueryIndex) -> 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 CBLQueryIndex) -> Self { + Self { cbl_ref } + } + //////// /// Returns the index's name. @@ -168,7 +174,13 @@ impl QueryIndex { /// Returns the collection that the index belongs to. pub fn collection(&self) -> Collection { - unsafe { Collection::retain(CBLQueryIndex_Collection(self.get_ref())) } + unsafe { Collection::reference(CBLQueryIndex_Collection(self.get_ref())) } + } +} + +impl Drop for QueryIndex { + fn drop(&mut self) { + unsafe { release(self.get_ref()) } } } @@ -280,7 +292,7 @@ impl Collection { let slice = from_str(name); let index = unsafe { CBLCollection_GetIndex(self.get_ref(), slice.get_ref(), &mut err) }; if !err { - return Ok(QueryIndex::retain(index)); + return Ok(QueryIndex::take_ownership(index)); } failure(err) } diff --git a/src/main.rs b/src/main.rs deleted file mode 100644 index 72b20e8..0000000 --- a/src/main.rs +++ /dev/null @@ -1,52 +0,0 @@ -// A very simple program using Couchbase Lite -// -// Copyright (c) 2020 Couchbase, Inc All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// -#![allow(deprecated)] - -extern crate couchbase_lite; -extern crate tempdir; - -use couchbase_lite::{ConcurrencyControl, Database, DatabaseConfiguration, Document, FleeceReference}; -use tempdir::TempDir; - -fn main() { - // Create a new database in a temporary directory: - let tmp_dir = TempDir::new("cbl_rust").expect("create temp dir"); - let cfg = DatabaseConfiguration { - directory: tmp_dir.path(), - #[cfg(feature = "enterprise")] - encryption_key: None, - }; - let mut db = Database::open("main_db", Some(cfg)).expect("open db"); - - // Create and save a new document: - { - //logging::set_level(logging::Level::Info, logging::Domain::All); - let mut doc = Document::new_with_id("foo"); - let mut props = doc.mutable_properties(); - props.at("i").put_i64(1234); - props.at("s").put_string("Hello World!"); - - db.save_document_with_concurency_control(&mut doc, ConcurrencyControl::FailOnConflict) - .expect("save"); - } - // Reload the document and verify its properties: - { - let doc = db.get_document("foo").expect("reload document"); - let props = doc.properties(); - assert_eq!(props.to_json(), r#"{"i":1234,"s":"Hello World!"}"#); - } -} diff --git a/src/query.rs b/src/query.rs index 8bdaa96..43cf8d4 100644 --- a/src/query.rs +++ b/src/query.rs @@ -48,7 +48,7 @@ unsafe extern "C" fn c_query_change_listener( token: *mut CBLListenerToken, ) { let callback = context as *const ChangeListener; - let query = Query::wrap(query.cast::()); + let query = Query::reference(query.cast::()); let token = ListenerToken::new(token); (*callback)(&query, &token); @@ -88,16 +88,22 @@ impl Query { return failure(err); } - Ok(Self { cbl_ref: q }) + Ok(Self::take_ownership(q)) } } - pub(crate) fn wrap(cbl_ref: *mut CBLQuery) -> 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 CBLQuery) -> 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 CBLQuery) -> Self { + Self { cbl_ref } + } + /** Assigns values to the query's parameters. These values will be substited for those parameters whenever the query is executed, until they are next assigned. diff --git a/src/replicator.rs b/src/replicator.rs index 00684c2..c265058 100644 --- a/src/replicator.rs +++ b/src/replicator.rs @@ -262,7 +262,7 @@ unsafe extern "C" fn c_replication_push_filter( flags: CBLDocumentFlags, ) -> bool { let repl_conf_context = context as *const ReplicationConfigurationContext; - let document = Document::retain(document.cast::()); + let document = Document::reference(document.cast::()); let (is_deleted, is_access_removed) = read_document_flags(flags); (*repl_conf_context) @@ -276,7 +276,7 @@ unsafe extern "C" fn c_replication_pull_filter( flags: CBLDocumentFlags, ) -> bool { let repl_conf_context = context as *const ReplicationConfigurationContext; - let document = Document::retain(document.cast::()); + let document = Document::reference(document.cast::()); let (is_deleted, is_access_removed) = read_document_flags(flags); (*repl_conf_context) @@ -307,12 +307,12 @@ unsafe extern "C" fn c_replication_conflict_resolver( let local_document = if local_document.is_null() { None } else { - Some(Document::retain(local_document as *mut CBLDocument)) + Some(Document::reference(local_document as *mut CBLDocument)) }; let remote_document = if remote_document.is_null() { None } else { - Some(Document::retain(remote_document as *mut CBLDocument)) + Some(Document::reference(remote_document as *mut CBLDocument)) }; (*repl_conf_context) diff --git a/src/scope.rs b/src/scope.rs index 7db8be4..ad9227f 100644 --- a/src/scope.rs +++ b/src/scope.rs @@ -19,12 +19,20 @@ pub struct Scope { impl Scope { pub const DEFAULT_NAME: &str = "_default"; - pub(crate) fn retain(cbl_ref: *mut CBLScope) -> Self { + //////// CONSTRUCTORS: + + /// Increase the reference counter of the CBL ref, so dropping the instance will NOT free the ref. + pub(crate) fn reference(cbl_ref: *mut CBLScope) -> 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 CBLScope) -> Self { + Self { cbl_ref } + } + /** Returns the name of the scope. */ pub fn name(&self) -> String { unsafe { @@ -36,7 +44,7 @@ impl Scope { /** Returns the scope's database */ pub fn database(&self) -> Database { - unsafe { Database::wrap(CBLScope_Database(self.get_ref())) } + unsafe { Database::reference(CBLScope_Database(self.get_ref())) } } /** Returns the names of all collections in the scope. */ @@ -63,7 +71,7 @@ impl Scope { if collection.is_null() { None } else { - Some(Collection::retain(collection)) + Some(Collection::take_ownership(collection)) } }) } @@ -84,6 +92,6 @@ impl Drop for Scope { impl Clone for Scope { fn clone(&self) -> Self { - Self::retain(self.get_ref()) + Self::reference(self.get_ref()) } } diff --git a/src/slice.rs b/src/slice.rs index 5e34f35..0f1637e 100644 --- a/src/slice.rs +++ b/src/slice.rs @@ -49,6 +49,7 @@ impl CblRef for Slice { } impl Slice { + /// Takes ownership of the slice, the reference counter is not increased so dropping the instance will free the ref. pub(crate) const fn wrap(slice: FLSlice, owner: T) -> Self { Self { cbl_ref: slice, diff --git a/tests/utils.rs b/tests/utils.rs index ddc2d84..8cac5ba 100644 --- a/tests/utils.rs +++ b/tests/utils.rs @@ -43,26 +43,29 @@ where { init_logging(); - let start_inst_count = instance_count() as isize; - let tmp_dir = TempDir::new("cbl_rust").expect("create temp dir"); - let cfg = DatabaseConfiguration { - directory: tmp_dir.path(), - #[cfg(feature = "enterprise")] - encryption_key: None, - }; - let mut db = Database::open(DB_NAME, Some(cfg)).expect("open db"); - assert!(Database::exists(DB_NAME, tmp_dir.path())); + let start_inst_count = instance_count(); - f(&mut db); + { + let tmp_dir = TempDir::new("cbl_rust").expect("create temp dir"); + let cfg = DatabaseConfiguration { + directory: tmp_dir.path(), + #[cfg(feature = "enterprise")] + encryption_key: None, + }; + let mut db = Database::open(DB_NAME, Some(cfg)).expect("open db"); + assert!(Database::exists(DB_NAME, tmp_dir.path())); + + f(&mut db); - db.delete().unwrap(); + db.delete().unwrap(); + } if LEAK_CHECK.is_some() { - warn!("Couchbase Lite objects were leaked by this test"); + info!("Checking if Couchbase Lite objects were leaked by this test"); dump_instances(); assert_eq!( - instance_count() as usize, - start_inst_count as usize, + instance_count(), + start_inst_count, "Native object leak: {} objects, was {}", instance_count(), start_inst_count