Adding ADR-0009

This revisits the decision in `ADR-0005` and explores using going back to handle maps to pass objects across the FFI.
mozilla · Dec 6, 2023 · 6307aa0 · 6307aa0
1 parent 9c353c5
commit 6307aa0
Show file tree

Hide file tree

Showing 3 changed files with 299 additions and 1 deletion.
diff --git a/docs/adr/0005-arc-pointers.md b/docs/adr/0005-arc-pointers.md
@@ -1,6 +1,6 @@
 # Use raw `Arc<T>` pointers to pass objects across the FFI
 
-* Status: proposed
+* Status: accepted
 * Deciders: mhammond, rfkelly
 * Consulted: travis, jhugman, dmose
 * Date: 2021-04-19

diff --git a/docs/adr/0009-handles.md b/docs/adr/0009-handles.md
@@ -0,0 +1,124 @@
+# Use handle map handles to pass objects across the FFI
+
+* Status: proposed
+* Deciders:
+* Consulted:
+
+Discussion and approval: 
+
+ADR-0005 discussion: [PR 430](https://github.com/mozilla/uniffi-rs/pull/430).
+
+## Context and Problem Statement
+
+UniFFI currently passes objects from Rust to the foreign side by leaking an Arc reference into a word-sized opaque pointer and passing it across the FFI.
+The basic approach uses `Arc::into_raw` / `Arc::from_raw` and was chosen in [ADR-0005](./0005-arc-pointers.md) for several reasons:
+
+  1. Clearer generated code.
+  2. Ability to pass objects as arguments (https://github.com/mozilla/uniffi-rs/issues/40).
+     This was deemed difficult to do with the existing codegen + HandleMap.
+  3. Ability to track object identity (https://github.com/mozilla/uniffi-rs/issues/197).  If two function calls return the same object, then this should result in an identical object on the foreign side.
+  4. Increased performance.
+
+Recently, this approach was extended to work with unsized types (`Arc<dyn Trait>`), which are normally wide pointers (i.e double-word sized).
+For these types, we box the Arc to create `Box<Arc<dyn Trait>>`, then leak the box pointer.
+This results in a regular, word-sized, pointer since `Arc<dyn Trait>` is sized (2 words) even when `dyn Trait` is not.
+
+Now that we have several years of experience, it's a good time to revisit some of the reasoning in ADR-0005 because it seems like we're not getting the benefits we wanted:
+
+* The code that deals with these isn't so clear, especially when we have to deal with unsized types (for example
+  the RustFuture
+  [allocation](https://github.com/mozilla/uniffi-rs/blob/fbc6631953a889c7af6e5f1af94de9242589b75b/uniffi_core/src/ffi/rustfuture/mod.rs#L56-L63) / [dellocation](https://github.com/mozilla/uniffi-rs/blob/fbc6631953a889c7af6e5f1af94de9242589b75b/uniffi_core/src/ffi/rustfuture/mod.rs#L124-L125) or the similar code for trait interfaces).
+* The codegen has progressed and it would be easy to support `[2]`.
+  We could simply `clone` the handle as part of the `lower()` call.
+* We've never implemented the reverse identity map needed for `[3]`.
+  The `NimbusClient` example given in https://github.com/mozilla/uniffi-rs/issues/419 would still fail today.
+  Given that there has been little to no demand for this feature, this should be changed to a non-goal.
+* The performance benefit decreases when discussing unsized types which require an additional layer of boxing.
+  In that case, instead of a strict decrease in work, we are trading a `HandleMap` insertion for a Box allocation.
+  This is a complex tradeoff, with the box allocation likely being faster, but not by much.
+
+Furthermore, practice has shown that dealing with raw pointers makes debugging difficult, with errors often resulting in segfaults or UB.
+Dealing with any sort of FFI handle is going to be error prone, but at least with a handle map we can generate better error messages and correct stack traces.
+There are also more error modes with this code.
+
+### Safety
+
+ADR-0005 says "We believe the additional safety offered by `HandleMap`s is far less important for this use-case, because the code using these pointers is generated instead of hand-written."
+
+While it's certainly true safety benefits matter less for generated code, it's also true that UniFFI is much more complex now then when ADR-0005 was decided.
+We have introduced callback interfaces, trait interfaces, Future handles for async functions, etc.
+All of these introduce additional failure cases, for example #1797, which means that relatively small safety benefits are more valuable.
+
+### Foreign handles
+
+A related question is how to handle handles to foreign objects that are passed into Rust.
+However, that question is orthogonal to this one and is out-of-scope for this ADR.
+
+## Considered Options
+
+### [Option 1] Continue using raw Arc pointers to pass Rust objects across the FFI
+
+Stay with the current status quo.
+
+### [Option 2] Use the old `HandleMap` to pass Rust objects across the FFI
+
+We could switch back to the old handle map code, which is still around in the [ffi-support crate](https://github.com/mozilla/ffi-support/blob/main/src/handle_map.rs).
+This implements a relatively simple handle-map that uses a `RWLock` to manage concurrency.
+
+See [../handles.md] for details on how this would work.
+
+Handles are passed as a `u64` values, but they only actually use 48 bits.
+This works better with JS, where the `Value` type only supports integers up to 53-bits wide.
+
+### [Option 3] Use a `HandleMap` with more performant/complex concurrency strategy
+
+We could switch to something like the [handle map implementation from #1808](https://github.com/bendk/uniffi-rs/blob/d305f7e47203b260e2e44009e37e7435fd554eaa/uniffi_core/src/ffi/slab.rs).
+The struct in that code was named `Slab` because it was inspired by the `tokio` `slab` crate.
+However, it's very similar to the original UniFFI `HandleMap` and this PR will call it a `HandleMap` to follow in that tradition.
+
+See [../handles.md] for details on how this would work.
+
+### [Option 4] Use a 3rd-party crate to pass Rust objects across the FFI
+
+We could also use a 3rd-party crate to handle this.
+The `sharded-slab` crate promises lock-free concurrency and supports generation counters.
+
+## Decision Drivers
+
+## Decision Outcome
+
+???
+
+## Pros and Cons of the Options
+
+### [Option 1] Continue using raw Arc pointers to pass Rust objects across the FFI
+
+* Good, because it has the fastest performance, especially for sized types.
+* Good, because it doesn't require code changes.
+* Bad, because it's hard to debug errors.
+
+### [Option 2] Use the original handle map to pass Rust objects across the FFI
+
+* Good, because it's easier to debug errors.
+* Bad, because it requires a read-write lock.
+  In particular, it seems bad that `insert`/`remove` can block `get`.
+* Good, because it works better with Javascript
+* Good, because it works with any type, not just `Arc<T>`.
+  For example, we might want to pass a handle to a [oneshot::Sender](https://docs.rs/oneshot/latest/oneshot/) across the FFI to implement async callback interface methods.
+
+### [Option 3] Use a handle map with a simpler concurrency strategy
+
+* Good, because it's easier to debug errors.
+* Good because `get` doesn't require a lock.
+* Bad because `insert` and `remove` requires a lock.
+* Bad, because it requires consumers to depend on `append-only-vec`.
+  However, this is a quite small crate.
+* Good, because it works better with Javascript
+* Good, because it works with any type, not just `Arc<T>`.
+
+### [Option 4] Use a 3rd-party crate to pass Rust objects across the FFI
+
+* Good, because it's easier to debug errors.
+* Bad, because it requires consumers to take this dependency.
+* Bad, because it makes it harder to implement custom functionality.
+  For example, supporting clone to fix https://github.com/mozilla/uniffi-rs/issues/1797 or adding a foreign bit to improve trait interface handling.
diff --git a/docs/handles.md b/docs/handles.md
@@ -0,0 +1,174 @@
+# How do UniFFI handles and handle maps work?
+
+UniFFI uses handles to pass Rust objects across the FFI to the foreign code.
+The handles point to an entry inside a `HandleMap`
+
+## HandleMap
+
+A `HandleMap` is a `Vec` where each item is either:
+
+- **Occupied**
+  - The foreign side holds a handle that's associated with the entry.
+  - Stores a `T` value (typically an `Arc<>`).
+  - Stores a `generation` counter used to detect use-after-free bugs
+  - Stores a `map-id` value used to detect handles used with the wrong `HandleMap`
+- **Vacant**
+  - Each vacant entry stores the index of the next vacant entry.
+    These form a linked-list of available entries and allow us to quickly allocate a new entry in the `HandleMap`
+  - Stores the `generation` counter value from the last time it was occupied, or 0 if it was never occupied.
+
+Furthermore, each `HandleMap` stores its own `next` value, which points to the first vacant entry on the free list.
+The value u32::MAX indicates indicates there is no next item and is represented by the `EOL` const.
+
+Here's an example `HandleMap`:
+
+```
+----------------------------------------------------------------
+| OCCUPIED           | VACANT             |  OCCUPIED          |
+| item: Foo          | next: EOL          |  item: Bar         |
+| generation: 100    | generation: 40     |  generation: 30    |
+----------------------------------------------------------------
+
+HandleMap.next: 1
+```
+
+### Inserting entries
+
+To insert a new entry:
+  - Remove the first entry in the free list, 
+  - Convert it to an `OCCUPIED`
+  - Increment the generation counter
+
+For example inserting a `Baz` entry in the above `HandleMap` would result in:
+
+```
+----------------------------------------------------------------
+| OCCUPIED           | OCCUPIED           |  OCCUPIED          |
+| item: Foo          | item: Baz          |  item: Bar         |
+| generation: 100    | generation: 41     |  generation: 30    |
+----------------------------------------------------------------
+
+HandleMap.next: EOL
+```
+
+If there are no vacant entries, then we append an entry to the end of the list.
+For example, inserting a `Qux` entry in the above `HandleMap` would result in:
+
+```
+-------------------------------------------------------------------------------------
+| OCCUPIED           | OCCUPIED           |  OCCUPIED          | OCCUPIED           |
+| item: Foo          | item: Baz          |  item: Bar         | item: Qux          |
+| generation: 100    | generation: 41     |  generation: 30    | generation: 0      |
+-------------------------------------------------------------------------------------
+
+HandleMap.next: EOL
+```
+
+### Removing entries
+
+To remove an entry:
+  - Convert it to `VACANT`
+  - Add it to the head of the free list.
+
+For example, removing the `Foo` entry from the above handle map would result in:
+
+```
+-------------------------------------------------------------------------------------
+| VACANT             | OCCUPIED           |  OCCUPIED          | OCCUPIED           |
+| next: EOL          | item: Baz          |  item: Bar         | item: Qux          |
+| generation: 100    | generation: 41     |  generation: 30    | generation: 0      |
+-------------------------------------------------------------------------------------
+
+HandleMap.next: 0
+```
+
+Removing the `Bar` entry after that would result in:
+
+```
+-------------------------------------------------------------------------------------
+| VACANT             | OCCUPIED           |  OCCUPIED          | OCCUPIED           |
+| next: EOL          | item: Baz          |  next: 0           | item: Qux          |
+| generation: 100    | generation: 41     |  generation: 30    | generation: 0      |
+-------------------------------------------------------------------------------------
+
+HandleMap.next: 2
+```
+
+### Getting entries
+
+When an entry is inserted, we return a `Handle`.
+This is a 64-bit integer, segmented as follows:
+- Bits 0-32: `Vec` index
+- Bit 32: foreign bit that's set for handles for foreign objects, but not Rust objects.
+  This allows us to differentiate trait interface implementations.
+- Bits 33-40: map id -- a unique value that corresponds to the map that generated the handle
+- Bits 40-48: generation counter
+- Bits 48-64: unused
+
+When the foreign code passes the Rust code a handle, we use it to get the entry as follows:
+
+- Use the index to get the entry in the raw `Vec`
+- Check that the entry is `OCCUPIED`, the generation counter value matches, and the map_id matches.
+- Get the stored item and do something with it.  Usually this means cloning the `Arc<>`.
+
+These checks can usually ensure that handles are only used with the `HandleMap` that generated them and that they aren't used after the entry is removed.
+However, this is limited by the bit-width of the handle segments:
+
+- Because the generation counter is 8-bits, we will fail to detect use-after-free bugs if an entry has been reused exactly 256 items or some multiple of 256.
+- Because the map id is only 7-bits, we may fail to detect handles being used with the wrong map if we generate over 128 `HandleMap` tables.
+  This can only happen if there more than 100 user-defined types and less than 1% of the time in that case.
+
+### Handle map creation / management
+
+The Rust codegen creates a static `HandleMap` for each object type that needs to be sent across the FFI, for example:
+  - `HandleMap<Arc<T>>` for each object type exposed by UniFFI.
+  - `HandleMap<Arc<dyn Trait>>` for each trait interface exposed by UniFFI.
+  - `HandleMap<Arc<dyn RustFutureFfi<FfiType>>>` for each FFI type.  This is used to implement async Rust functions.
+  - `HandleMap<oneshot::Sender<FfiType>>` for each FFI type.  This will be used to implement async callback methods.
+
+The `HandleAlloc` trait manages access to the static `HandleMap` instances and provides the following methods:
+  - `insert(value: Self) -> Handle` insert a new entry into the handle map
+  - `remove(handle: Handle) -> Self` remove an entry from the handle map
+  - `get(handle: Handle) -> Self` get a cloned object from the handle map without removing the entry.
+  - `clone_handle(handle: Handle) -> Handle` get a cloned handle that refers to the same object.
+
+If the user defines a type `Foo` in their interface then:
+ - `<Arc<Foo> as HandleAlloc>::insert` is called when lowering `Foo` to pass it across the FFI to the foreign side.
+ - `<Arc<Foo> as HandleAlloc>::get` is called when lifting `Foo` after it's passed back across the FFI to Rust.
+ - `<Arc<Foo> as HandleAlloc>::clone_handle` is called when the foreign side needs to clone the handle.
+   See https://github.com/mozilla/uniffi-rs/issues/1797 for why this is needed.
+ - `<Arc<Foo> as HandleAlloc>::remove` is called when the foreign side calls the `free` function for the object.
+
+Extra details:
+  - The trait is actually `HandleAlloc<UT>`, where `UT` is the "UniFFI Tag" type. See `uniffi_core/src/ffi_converter_traits.rs` for details.
+  - The last two `HandleAlloc` methods are only implemented for `T: Clone`, which is true for the `Arc<>` cases, but not `oneshot::Sender`.
+    This is fine because we only use `insert`/`remove` for the `oneshot::Sender` case.
+
+### Concurrency
+
+`insert` and `remove` require serialization since there's no way to atomically update the free list.
+In general, `get` does not require any serialization since it will only read occupied entries, while `insert` and `remove` only access vacant entries.
+However, there are 2 edge cases where `get` does access the same entries as `insert`/`remove`:
+
+  - If `insert` causes the Vec to grow this may cause the entire array to be moved, which will affect `get`
+  - If the foreign code has a use-after-free bug, then `get` may access the same entry as an `insert`/`remove` operation.
+
+UniFFI uses the following system to handle this:
+
+- A standard `Mutex` is used to serialize `insert` and `remove`.
+- We use the `append_only_vec` crate, which avoids moving the array when the `Vec` grows.
+- Each entry has a 8-bit read-write spin-lock to avoid issues in the face of use-after-free bugs.
+  This lock will only be contested if there's a use-after-free bug.
+
+### Concurrency: alternative option if we choose 2 from the ADR.  This one is simpler, but slower.
+
+To allow concurrent access, a `RwLock` is used to protect the entire `HandleMap`.
+`insert` and `remove` acquire the write lock while accessing entries acquires the read lock.
+
+### Space usage
+
+The `HandleMap` adds an extra 64-bits of memory to each occupied item, which is the lower-limit on a 64-bit machine.
+This means that `HandleMap` tables that store normal `Arc` pointers add ~100% extra space overhead and ones that store wide-pointers add ~50% overhead.
+`HandleMap` tables don't have any way of reclaiming unused space after items are removed.
+
+This is can be a very large amount of memory, but in practice libraries only generate a relatively small amount of handles.