Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Experiment with using raw pointers for objects. #366

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/manual/book.toml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
[book]
authors = ["Edouard Oger"]
authors = ["Edouard Oger", "Ryan Kelly"]
language = "en"
multilingual = false
src = "src"
Expand Down
6 changes: 6 additions & 0 deletions docs/manual/src/SUMMARY.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,9 @@ Hello everyone
# Swift

- [Integrating with XCode](./swift/xcode.md)

# Internals
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much like our lifting/lowering/serialization story, the way we currently use handlemaps is under-documented. Let's add a new section to the manual that we can use for narrative docs about such things.


- [Lifting, Lowering, and Serialization](./internals/lifting_and_lowering.md)
- [Serialization format](./internals/serialization_format.md)
- [Managing object references](./internals/object_references.md)
3 changes: 3 additions & 0 deletions docs/manual/src/internals/lifting_and_lowering.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Lifting, Lowering and Serialization

Here is where I will write about lifting and lowering.
3 changes: 3 additions & 0 deletions docs/manual/src/internals/object_references.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Managing Object References

Here is where I'll talk about handlemaps, and about the upcoming "threadsafe" mode.
3 changes: 3 additions & 0 deletions docs/manual/src/internals/serialization_format.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Serialization Format

Here is where I will describe our incredibly basic serialization format.
106 changes: 106 additions & 0 deletions docs/manual/src/udl/interfaces.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,4 +71,110 @@ func display(list: TodoListProtocol) {
print($0)
}
}
```

# Concurrent Access
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on this experiment and subsequent conversations, here's a concrete proposal for how to expose uniffi interfaces in a way that gives us more control over the concurrency story. Feedback most welcome at this stage! /cc @mhammond @jhugman @dmose.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Note that the PR doesn't actually implement proposal yet, although I'm confident that it can be implemented without much fuss)

Copy link
Member

@mhammond mhammond Jan 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems awesome. While it doesn't handle the "multiple bindings" use-case, I wonder if we can avoid uniffi knowing about that kind of setup? Eg, I wonder if we can arrange so that instead of lib.rs having, say:

    pub fn get_active_experiments(&self) -> Result<Vec<EnrolledExperiment>> {
        get_enrollments(self.db()?)
    }

instead it does something like:

    pub fn get_active_experiments(&self) -> Result<Vec<EnrolledExperiment>> {
       handwavey_get_static_instance().get_active_experiments()
    }

(ie, that it's OK if there are multiple instances of the bindings, because they all just delegate). I'd need to play a little to see if that might work in practice though. Did you have anything in mind for that use-case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(We discussed this a little in meetings and slack, but commenting here for posterity)

I'd be interested in exposing the singleton concept more directly through the IDL. For example, imagine a Nimbus API definition that looks like this:

namespace nimbus {
  // Returns the global NimbusClient singleton, or null if it has not been initialized yet.
  NimbusClient? get_global_nimbus_client();

  // Sets the global NimbusClient singleton, or throws if it has already been set.
  [Throws=Error]
  void set_global_nimbus_client(NimbusClient client);
};

interface NimbusClient {
  // all the usual methods here
}

Uniffi doesn't support this today. Specifically, we don't let you pass object references as arguments (#40) or return them from functions (#197), so you wouldn't be able to declare the getter/setter for the singleton.

However, I think it's clear that we want to support such functionality at some point, and I don't think there's any deep technical reason why we couldn't do so, there's just a bunch of details to carefully work through (as described in the linked issues).


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]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bikeshedding also welcome on how to expose this in the UDL...

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<u64>
}

impl Counter {
fn new() -> Self {
// `RefCell` is not `Sync`, so neither is `Counter`.
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 an `AtomicU64` for interior mutability, which is both `Sync` and
`Send` and hence will compile successfully:

```rust
struct Counter {
value: AtomicU64
}

impl Counter {
fn new() -> Self {
Self { value: AtomicU64::new(0) }
}

fn increment(&self) {
self.value.fetch_add(1, Ordering::SeqCst);
}

fn get(&self) -> u64 {
self.value.load(Ordering::SeqCst)
}
}
```
6 changes: 3 additions & 3 deletions examples/rondpoint/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ fn switcheroo(b: bool) -> bool {
// Even if roundtripping works, it's possible we have
// symmetrical errors that cancel each other out.
#[derive(Debug, Clone)]
struct Retourneur;
pub struct Retourneur;
impl Retourneur {
fn new() -> Self {
Retourneur
Expand Down Expand Up @@ -129,7 +129,7 @@ impl Retourneur {
}

#[derive(Debug, Clone)]
struct Stringifier;
pub struct Stringifier;

#[allow(dead_code)]
impl Stringifier {
Expand Down Expand Up @@ -175,7 +175,7 @@ impl Stringifier {
}

#[derive(Debug, Clone)]
struct Optionneur;
pub struct Optionneur;
impl Optionneur {
fn new() -> Self {
Optionneur
Expand Down
2 changes: 1 addition & 1 deletion examples/sprites/tests/test_generated_bindings.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
uniffi_macros::build_foreign_language_testcases!(
"src/sprites.udl",
[
// "tests/bindings/test_sprites.py",
"tests/bindings/test_sprites.py",
"tests/bindings/test_sprites.kts",
"tests/bindings/test_sprites.swift",
]
Expand Down
2 changes: 1 addition & 1 deletion examples/todolist/tests/test_generated_bindings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,6 @@ uniffi_macros::build_foreign_language_testcases!(
[
"tests/bindings/test_todolist.kts",
"tests/bindings/test_todolist.swift",
// "tests/bindings/test_todolist.py"
"tests/bindings/test_todolist.py"
]
);
1 change: 1 addition & 0 deletions uniffi/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
1 change: 1 addition & 0 deletions uniffi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions uniffi_bindgen/src/bindings/gecko_js/gen_gecko_js.rs
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,7 @@ mod filters {
FFIType::UInt64 => "uint64_t".into(),
FFIType::Float32 => "float".into(),
FFIType::Float64 => "double".into(),
FFIType::Pointer(_) => "void*".into(), // TODO: name the type
FFIType::RustCString => "const char*".into(),
FFIType::RustBuffer => context.ffi_rustbuffer_type(),
FFIType::RustError => context.ffi_rusterror_type(),
Expand Down
1 change: 1 addition & 0 deletions uniffi_bindgen/src/bindings/kotlin/gen_kotlin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ mod filters {
FFIType::Int64 | FFIType::UInt64 => "Long".to_string(),
FFIType::Float32 => "Float".to_string(),
FFIType::Float64 => "Double".to_string(),
FFIType::Pointer(_) => "Pointer".to_string(), // TODO: name the type?
FFIType::RustCString => "Pointer".to_string(),
FFIType::RustBuffer => "RustBuffer.ByValue".to_string(),
FFIType::RustError => "RustError".to_string(),
Expand Down
12 changes: 6 additions & 6 deletions uniffi_bindgen/src/bindings/kotlin/templates/Helpers.kt
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,16 @@
// This would be a good candidate for isolating in its own ffi-support lib.

abstract class FFIObject(
private val handle: AtomicLong
private val pointer: AtomicReference<Pointer?>
) {
open fun destroy() {
this.handle.set(0L)
this.pointer.set(null)
}

internal inline fun <R> callWithHandle(block: (handle: Long) -> R) =
this.handle.get().let { handle ->
if (handle != 0L) {
block(handle)
internal inline fun <R> callWithPointer(block: (pointer: Pointer) -> R) =
this.pointer.get().let { pointer ->
if (pointer != null) {
block(pointer)
} else {
throw IllegalStateException("${this.javaClass.simpleName} object has already been destroyed")
}
Expand Down
16 changes: 8 additions & 8 deletions uniffi_bindgen/src/bindings/kotlin/templates/ObjectTemplate.kt
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ public interface {{ obj.name()|class_name_kt }}Interface {
}

class {{ obj.name()|class_name_kt }}(
handle: Long
) : FFIObject(AtomicLong(handle)), {{ obj.name()|class_name_kt }}Interface {
ptr: Pointer
) : FFIObject(AtomicReference(ptr)), {{ obj.name()|class_name_kt }}Interface {

{%- for cons in obj.constructors() %}
constructor({% call kt::arg_list_decl(cons) -%}) :
Expand All @@ -19,16 +19,16 @@ class {{ obj.name()|class_name_kt }}(

/**
* Disconnect the object from the underlying Rust object.
*
*
* It can be called more than once, but once called, interacting with the object
* causes an `IllegalStateException`.
*
*
* Clients **must** call this method once done with the object, or cause a memory leak.
*/
override fun destroy() {
try {
callWithHandle {
super.destroy() // poison the handle so no-one else can use it before we tell rust.
callWithPointer {
super.destroy() // poison the pointer so no-one else can use it before we tell rust.
rustCall(InternalError.ByReference()) { err ->
_UniFFILib.INSTANCE.{{ obj.ffi_object_free().name() }}(it, err)
}
Expand All @@ -43,15 +43,15 @@ class {{ obj.name()|class_name_kt }}(

{%- when Some with (return_type) -%}
override fun {{ meth.name()|fn_name_kt }}({% call kt::arg_list_protocol(meth) %}): {{ return_type|type_kt }} =
callWithHandle {
callWithPointer {
{%- call kt::to_ffi_call_with_prefix("it", meth) %}
}.let {
{{ "it"|lift_kt(return_type) }}
}

{%- when None -%}
override fun {{ meth.name()|fn_name_kt }}({% call kt::arg_list_protocol(meth) %}) =
callWithHandle {
callWithPointer {
{%- call kt::to_ffi_call_with_prefix("it", meth) %}
}
{% endmatch %}
Expand Down
2 changes: 1 addition & 1 deletion uniffi_bindgen/src/bindings/kotlin/templates/wrapper.kt
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import com.sun.jna.Pointer
import com.sun.jna.Structure
import java.nio.ByteBuffer
import java.nio.ByteOrder
import java.util.concurrent.atomic.AtomicLong
import java.util.concurrent.atomic.AtomicReference

{% include "RustBufferTemplate.kt" %}

Expand Down
1 change: 1 addition & 0 deletions uniffi_bindgen/src/bindings/python/gen_python.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ mod filters {
FFIType::UInt64 => "ctypes.c_uint64".to_string(),
FFIType::Float32 => "ctypes.c_float".to_string(),
FFIType::Float64 => "ctypes.c_double".to_string(),
FFIType::Pointer(_) => "ctypes.c_voidp".to_string(), // TODO: name the type?
FFIType::RustCString => "ctypes.c_voidp".to_string(),
FFIType::RustBuffer => "RustBuffer".to_string(),
FFIType::RustError => "ctypes.POINTER(RustError)".to_string(),
Expand Down
1 change: 1 addition & 0 deletions uniffi_bindgen/src/bindings/swift/gen_swift.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ mod filters {
FFIType::UInt64 => "uint64_t".into(),
FFIType::Float32 => "float".into(),
FFIType::Float64 => "double".into(),
FFIType::Pointer(_) => "void*_Nonnull".into(), // TODO: name the type?
FFIType::RustCString => "const char*_Nonnull".into(),
FFIType::RustBuffer => "RustBuffer".into(),
FFIType::RustError => "NativeRustError".into(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,17 @@ public protocol {{ obj.name() }}Protocol {
}

public class {{ obj.name() }}: {{ obj.name() }}Protocol {
private let handle: UInt64
private let pointer: UnsafeMutableRawPointer

{%- for cons in obj.constructors() %}
public init({% call swift::arg_list_decl(cons) -%}) {% call swift::throws(cons) %} {
self.handle = {% call swift::to_ffi_call(cons) %}
self.pointer = {% call swift::to_ffi_call(cons) %}
}
{%- endfor %}

deinit {
try! rustCall(InternalError.unknown()) { err in
{{ obj.ffi_object_free().name() }}(handle, err)
{{ obj.ffi_object_free().name() }}(pointer, err)
}
}

Expand All @@ -30,13 +30,13 @@ public class {{ obj.name() }}: {{ obj.name() }}Protocol {

{%- when Some with (return_type) -%}
public func {{ meth.name()|fn_name_swift }}({% call swift::arg_list_decl(meth) %}) {% call swift::throws(meth) %} -> {{ return_type|type_swift }} {
let _retval = {% call swift::to_ffi_call_with_prefix("self.handle", meth) %}
let _retval = {% call swift::to_ffi_call_with_prefix("self.pointer", meth) %}
return {% call swift::try(meth) %} {{ "_retval"|lift_swift(return_type) }}
}

{%- when None -%}
public func {{ meth.name()|fn_name_swift }}({% call swift::arg_list_decl(meth) %}) {% call swift::throws(meth) %} {
{% call swift::to_ffi_call_with_prefix("self.handle", meth) %}
{% call swift::to_ffi_call_with_prefix("self.pointer", meth) %}
}
{%- endmatch %}
{% endfor %}
Expand Down
Loading