From d5339e671967b6a77ada10b6e027da8bfafbb42a Mon Sep 17 00:00:00 2001 From: Ryan Kelly Date: Tue, 12 Jan 2021 17:31:48 +1100 Subject: [PATCH] Sketch out an actual proposed solution for non-blocking concurrency. --- docs/manual/book.toml | 2 +- docs/manual/src/SUMMARY.md | 6 + .../src/internals/lifting_and_lowering.md | 3 + .../manual/src/internals/object_references.md | 3 + .../src/internals/serialization_format.md | 3 + docs/manual/src/udl/interfaces.md | 110 +++++++++++++++++- uniffi/Cargo.toml | 1 + uniffi/src/lib.rs | 1 + .../src/templates/ObjectTemplate.rs | 3 + uniffi_bindgen/src/templates/macros.rs | 8 +- 10 files changed, 134 insertions(+), 6 deletions(-) create mode 100644 docs/manual/src/internals/lifting_and_lowering.md create mode 100644 docs/manual/src/internals/object_references.md create mode 100644 docs/manual/src/internals/serialization_format.md diff --git a/docs/manual/book.toml b/docs/manual/book.toml index fa8c8d741c..97d0ec0441 100644 --- a/docs/manual/book.toml +++ b/docs/manual/book.toml @@ -1,5 +1,5 @@ [book] -authors = ["Edouard Oger"] +authors = ["Edouard Oger", "Ryan Kelly"] language = "en" multilingual = false src = "src" diff --git a/docs/manual/src/SUMMARY.md b/docs/manual/src/SUMMARY.md index 6862a16d4c..ca5083caea 100644 --- a/docs/manual/src/SUMMARY.md +++ b/docs/manual/src/SUMMARY.md @@ -24,3 +24,9 @@ Hello everyone # Swift - [Integrating with XCode](./swift/xcode.md) + +# Internals + +- [Lifting, Lowering, and Serialization](./internals/lifting_and_lowering.md) + - [Serialization format](./internals/serialization_format.md) +- [Managing object references](./internals/object_references.md) \ No newline at end of file diff --git a/docs/manual/src/internals/lifting_and_lowering.md b/docs/manual/src/internals/lifting_and_lowering.md new file mode 100644 index 0000000000..5c43a355c5 --- /dev/null +++ b/docs/manual/src/internals/lifting_and_lowering.md @@ -0,0 +1,3 @@ +# Lifting, Lowering and Serialization + +Here is where I will write about lifting and lowering. \ No newline at end of file diff --git a/docs/manual/src/internals/object_references.md b/docs/manual/src/internals/object_references.md new file mode 100644 index 0000000000..fc7a1cf0f5 --- /dev/null +++ b/docs/manual/src/internals/object_references.md @@ -0,0 +1,3 @@ +# Managing Object References + +Here is where I'll talk about handlemaps, and about the upcoming "threadsafe" mode. \ No newline at end of file diff --git a/docs/manual/src/internals/serialization_format.md b/docs/manual/src/internals/serialization_format.md new file mode 100644 index 0000000000..169774ae8a --- /dev/null +++ b/docs/manual/src/internals/serialization_format.md @@ -0,0 +1,3 @@ +# Serialization Format + +Here is where I will describe our incredibly basic serialization format. \ No newline at end of file diff --git a/docs/manual/src/udl/interfaces.md b/docs/manual/src/udl/interfaces.md index 767670066c..3f100afeda 100644 --- a/docs/manual/src/udl/interfaces.md +++ b/docs/manual/src/udl/interfaces.md @@ -71,4 +71,112 @@ func display(list: TodoListProtocol) { print($0) } } -``` \ No newline at end of file +``` + +# Concurrent Access + +Since interfaces represent mutable data, uniffi has to take extra care +to uphold Rust's safety guarantees around shared and mutable references. +The foreign-language code may attempt to operate on an interface instance +from multiple threads, and it's important that this not violate Rust's +assumption that there is at most a single mutable reference to a struct +at any point in time. + +By default, uniffi enforces this using runtime locking. Each interface instance +has an associated lock which is transparently acquired at the beginning of each +call to a method of that instance, and released once the method returns. This +approach is simple and safe, but it means that all method calls on an instance +are run in a strictly sequential fashion, limiting concurrency. + +You can opt out of this protection by marking the interface as threadsafe: + +```idl +[Threadsafe] +interface Counter { + constructor(); + void increment(); + u64 get(); +}; +``` + +The uniffi-generated code will allow concurrent method calls on threadsafe interfaces +without any locking. + +For this to be safe, the underlying Rust struct must adhere to certain restrictions, and +uniffi's generated Rust scaffolding will emit compile-time errors if it does not. + +The Rust struct must not expose any methods that take `&mut self`. The following implementation +of the `Counter` interface will fail to compile because it relies on mutable references: + +```rust +struct Counter { + value: u64 +} + +impl Counter { + fn new() -> Self { + Self { value: 0 } + } + + // No mutable references to self allowed in [Threadsafe] interfaces. + fn increment(&mut self) { + self.value = self.value + 1; + } + + fn get(&self) -> u64 { + self.value + } +} +``` + +Implementations can instead use Rust's "interior mutability" pattern. However, they +must do so in a way that is both `Sync` and `Send`, since the foreign-language code +may operate on the instance from multiple threads. The following implementation of the +`Counter` interface will fail to compile because `RefCell` is not `Send`: + +```rust +struct Counter { + value: RefCell +} + +impl Counter { + fn new() -> Self { + Self { value: RefCell::new(0) } + } + + fn increment(&self) { + let mut value = self.value.borrow_mut(); + *value = *value + 1; + } + + fn get(&self) -> u64 { + *self.value.borrow() + } +} +``` + +This version uses the `AtomicU64` struct for interior mutability, which is both `Sync` and +`Send` and hence will compile successfully: + +```rust +struct Counter { + value: i64 +} + +impl Counter { + fn new() -> Self { + Self { 0 } + } + + fn increment(&self) { + self.value = self.value + 1; + } + + fn get(&self) -> i64 { + self.value + } +} +``` + +Uniffi aims to uphold Rust's safety guarantees at all times, without requiring the +foreign-language code to know or care about them. \ No newline at end of file diff --git a/uniffi/Cargo.toml b/uniffi/Cargo.toml index 5d5608e842..a307fa7c6c 100644 --- a/uniffi/Cargo.toml +++ b/uniffi/Cargo.toml @@ -17,6 +17,7 @@ bytes = "0.5" ffi-support = "~0.4.2" lazy_static = "1.4" log = "0.4" +static_assertions = "1" # Regular dependencies cargo_metadata = "0.11" paste = "1.0" diff --git a/uniffi/src/lib.rs b/uniffi/src/lib.rs index ac7ec6de54..5cf1383084 100644 --- a/uniffi/src/lib.rs +++ b/uniffi/src/lib.rs @@ -47,6 +47,7 @@ pub mod deps { pub use ffi_support; pub use lazy_static; pub use log; + pub use static_assertions; } /// Trait defining how to transfer values via the FFI layer. diff --git a/uniffi_bindgen/src/templates/ObjectTemplate.rs b/uniffi_bindgen/src/templates/ObjectTemplate.rs index f758a3ae13..322c17e8cd 100644 --- a/uniffi_bindgen/src/templates/ObjectTemplate.rs +++ b/uniffi_bindgen/src/templates/ObjectTemplate.rs @@ -22,6 +22,9 @@ unsafe impl uniffi::deps::ffi_support::IntoFfi for {{ obj.name() }} { fn into_ffi_value(self) -> Self::Value { Some(Box::new(self)) } } +// For thread-safety, we only support raw pointers on things that are Sync and Send. +uniffi::deps::static_assertions::assert_impl_all!({{ obj.name() }}: Sync, Send); + #[no_mangle] pub extern "C" fn {{ obj.ffi_object_free().name() }}(obj : Option>) { drop(obj); diff --git a/uniffi_bindgen/src/templates/macros.rs b/uniffi_bindgen/src/templates/macros.rs index 68cd0ff6d5..8200d6bdf3 100644 --- a/uniffi_bindgen/src/templates/macros.rs +++ b/uniffi_bindgen/src/templates/macros.rs @@ -55,9 +55,9 @@ uniffi::deps::ffi_support::call_with_output(err, || { uniffi::deps::ffi_support::call_with_result(err, || -> Result<{% call return_type_func(meth) %}, {{e}}> { // We're declaring the argument type as an `Option>` but the value is owned by the foreign code, // we so don't want to drop the Box. Probably it would be better to encode this as a reference type. - let mut obj_box = std::mem::ManuallyDrop::new(obj.expect("Must receive a non-null object pointer")); + let obj_box = std::mem::ManuallyDrop::new(obj.expect("Must receive a non-null object pointer")); // TODO: terrifically unsafe to assume we can take a mutable reference here! Needs locks etc. - let obj = obj_box.as_mut(); + let obj = obj_box.as_ref(); let _retval = {{ obj.name() }}::{%- call to_rs_call_with_prefix("obj", meth) -%}?; Ok({% call ret(meth) %}) }) @@ -65,9 +65,9 @@ uniffi::deps::ffi_support::call_with_result(err, || -> Result<{% call return_typ uniffi::deps::ffi_support::call_with_output(err, || { // We're declaring the argument type as an `Option>` but the value is owned by the foreign code, // we so don't want to drop the Box. Probably it would be better to encode this as a reference type. - let mut obj_box = std::mem::ManuallyDrop::new(obj.expect("Must receive a non-null object pointer")); + let obj_box = std::mem::ManuallyDrop::new(obj.expect("Must receive a non-null object pointer")); // TODO: terrifically unsafe to assume we can take a mutable reference here! Needs locks etc. - let obj = obj_box.as_mut(); + let obj = obj_box.as_ref(); let _retval = {{ obj.name() }}::{%- call to_rs_call_with_prefix("obj", meth) -%}; {% call ret(meth) %} })