Better system for handles #1730

bendk · 2023-09-05T15:29:18Z

There are many situations where we manage opaque FFI handles: callback interfaces, futures, callback data, etc. The basic requirement is to create a handle for an object, pass it across the FFI, use the handle for FFI calls, then finally pass the handle to some function to free the object.

We generally use pointer/usize handles for this and have several ad-hoc systems to handle it. There are several issues with these systems:

Pointers vary in size between platforms
Pointers require extra level of boxing when used with Arc<dyn Trait> (and currently oneshot::Sender)
Not all foreign languages support pointers directly (JS even has issues with 64-bit integers)
Managing the reference count in the foreign language is hard

I think we can come up with a general system for handles that avoids these issues. The starting point can be the slab crate, which is pretty simple and essentially creates usize handles to manage object allocations. I think we can update it to use u32 handles and catch use-after-free errors in some case (maybe most).

The text was updated successfully, but these errors were encountered:

The initial motivation for this was cancellation. PR#1697 made it so if an async function was cancelled we would eventually release the resources. However, it would be better if we could immedately release the resources. In order to implement that, I realized I needed to change the future FFI quite a bit. The new FFI is simpler overall and supports cancel and drop operations. Cancel ensures that the foreign code will resume and break out of its async code. Drop ensures that all resources from the wrapped future are relased. The new code does not use ForeignExecutor and that code is in a state of limbo for now. I hope to repurpose it for foreign dispatch queues (mozilla#1734). If that doesn't work out, we can just delete it. It's tricky to implement FFI calls that inputted type-erased `RustFutureHandle`, since the `F` parameter is an anonymous type that implements Future. Made a new system that is based to converting an `Arc<RustFuture<F, T, U>>` into a `Box<Arc<dyn RustFutureFFI>>` before sending it across the FFI. `Arc<dyn RustFutureFFI>` implements the FFI and the extra Box converts the wide pointer into a normal pointer. It's fairly messy, but I hope mozilla#1730 will improve things. - Updated the futures fixture tests for this to hold on to the mutex longer in the initial call. This makes it so they will fail unless the future is dropped while the mutex is still locked. Before they would only succeed as long as the mutex was dropped once the timeout expired. - Updated `RustCallStatus.code` field to be an enum. Added `Cancelled` as one of the variants. `Cancelled` is only used for async functions. - Removed the FutureCallback and invoke_future_callback from `FfiConverter`. - New syncronization handling code in RustFuture that's hopefully clearer, more correct, and more understandable than the old stuff. - Updated `UNIFFI_CONTRACT_VERSION` since this is an ABI change - Removed the `RustCallStatus` param from async scaffolding functions. These functions can't fail, so there's no need.

- Updated the continuation handling code to use a Mutex rather than hand-coded locking code. The performance will be slightly worse, but the code simplicity is worth it, and I think I can improve performance when implementing mozilla#1730. - Register the continuation callback at startup rather than passing around each call. Use the callback cell code that I added for this purpose, but forgot to use.

This adds support for the foreign bindings side to implement Rust traits and for both sides to pass `Arc<dyn [Trait]>` objects back and forth. I thought this would be a good time to redo the callback interface code. The trait interface code uses a foreign-supplied vtable rather than a single callback and method indexes like the old code used. Renamed `CALL_PANIC` to `CALL_UNEXPECTED_ERROR` in the foreign bindings templates. This matches the name in the Rust code and makes more sense for foreign trait implementations. -------------------- TODO ----------------------------- This currently requires "wrapping" the object every time it's passed across the FFI. If the foreign code receives a trait object, then passes it back to Rust. Rust now has a handle to the foreign impl and that foreign impl just calls back into Rust. I think mozilla#1730 could help solve this. I think there should be better tests for reference counts, but I'm waiting until we address the previous issue to implement them. Python requires a hack to return RustBuffer from callbacks (RubiconCtypesPatch.py) because of an old ctypes bug (https://bugs.python.org/issue5710). The options I can see are: - Go back to the callback interface style, where we input a RustBuffer and write the output to it. - Pass the return type as an out pointer. This requires some special handling for void returns though. - Implement mozilla#1779, which changes RustBuffer to a simple `*mut u8` and then Python could return it. This is my preference, I don't like the idea of making the ABI worse just because of Python. TODO: If I make the Python tests fail, I see `RuntimeWarning: memory leak in callback function` in the console. I believe this is because of the rubicon hack, but I'm not sure. We should double check this before merging. Kotlin is not implemented yet. I think when we do implement it, we're going to want to face mozilla#1394 and make a decision there. I don't think it's reasonable to have consumers have to call a `close()` method on these interfaces. I also don't know how we would deal with this in Rust. ISTM that the same logic that says Kotlin objects need to be manually closed dictates that if you use a Kotlin object in Rust, there should be a `close()` method or something analogous to a to `AutoCloseable`. The Ruby coverall tests are now failing because they don't implement trait interfaces. We need to figure out a way for them to pass the test suite.

This adds support for the foreign bindings side to implement Rust traits and for both sides to pass `Arc<dyn [Trait]>` objects back and forth. I thought this would be a good time to redo the callback interface code. The trait interface code uses a foreign-supplied vtable rather than a single callback and method indexes like the old code used. Renamed `CALL_PANIC` to `CALL_UNEXPECTED_ERROR` in the foreign bindings templates. This matches the name in the Rust code and makes more sense for foreign trait implementations. -------------------- TODO ----------------------------- This currently requires "wrapping" the object every time it's passed across the FFI. If the foreign code receives a trait object, then passes it back to Rust. Rust now has a handle to the foreign impl and that foreign impl just calls back into Rust. I think mozilla#1730 could help solve this. I think there should be better tests for reference counts, but I'm waiting until we address the previous issue to implement them. Python requires a hack to return RustBuffer from callbacks (RubiconCtypesPatch.py) because of an old ctypes bug (https://bugs.python.org/issue5710). The options I can see are: - Go back to the callback interface style, where we input a RustBuffer and write the output to it. - Pass the return type as an out pointer. This requires some special handling for void returns though. - Implement mozilla#1779, which changes RustBuffer to a simple `*mut u8` and then Python could return it. This is my preference, I don't like the idea of making the ABI worse just because of Python. TODO: If I make the Python tests fail, I see `RuntimeWarning: memory leak in callback function` in the console. I believe this is because of the rubicon hack, but I'm not sure. We should double check this before merging. Kotlin is not implemented yet. I think when we do implement it, we're going to want to face mozilla#1394 and make a decision there. I don't think it's reasonable to have consumers have to call a `close()` method on these interfaces. I also don't know how we would deal with this in Rust. ISTM that the same logic that says Kotlin objects need to be manually closed dictates that if you use a Kotlin object in Rust, there should be a `close()` method or something analogous to a to `AutoCloseable`. The Ruby coverall tests are now failing because they don't implement trait interfaces. We need to figure out a way for them to pass the test suite. Remove all the debugging printouts

This adds support for the foreign bindings side to implement Rust traits and for both sides to pass `Arc<dyn [Trait]>` objects back and forth. I thought this would be a good time to redo the callback interface code. The trait interface code uses a foreign-supplied vtable rather than a single callback and method indexes like the old code used. Renamed `CALL_PANIC` to `CALL_UNEXPECTED_ERROR` in the foreign bindings templates. This matches the name in the Rust code and makes more sense for foreign trait implementations. -------------------- TODO ----------------------------- This currently requires "wrapping" the object every time it's passed across the FFI. If the foreign code receives a trait object, then passes it back to Rust. Rust now has a handle to the foreign impl and that foreign impl just calls back into Rust. I think mozilla#1730 could help solve this. I think there should be better tests for reference counts, but I'm waiting until we address the previous issue to implement them. Python requires a hack to return RustBuffer from callbacks (RubiconCtypesPatch.py) because of an old ctypes bug (https://bugs.python.org/issue5710). The options I can see are: - Go back to the callback interface style, where we input a RustBuffer and write the output to it. - Pass the return type as an out pointer. This requires some special handling for void returns though. - Implement mozilla#1779, which changes RustBuffer to a simple `*mut u8` and then Python could return it. This is my preference, I don't like the idea of making the ABI worse just because of Python. TODO: If I make the Python tests fail, I see `RuntimeWarning: memory leak in callback function` in the console. I believe this is because of the rubicon hack, but I'm not sure. We should double check this before merging. Kotlin is not implemented yet. I think when we do implement it, we're going to want to face mozilla#1394 and make a decision there. I don't think it's reasonable to have consumers have to call a `close()` method on these interfaces. I also don't know how we would deal with this in Rust. ISTM that the same logic that says Kotlin objects need to be manually closed dictates that if you use a Kotlin object in Rust, there should be a `close()` method or something analogous to a to `AutoCloseable`. The Ruby coverall tests are now failing because they don't implement trait interfaces. We need to figure out a way for them to pass the test suite. Remove all the debugging printouts Document this

This adds support for the foreign bindings side to implement Rust traits and for both sides to pass `Arc<dyn [Trait]>` objects back and forth. I thought this would be a good time to redo the callback interface code. The trait interface code uses a foreign-supplied vtable rather than a single callback and method indexes like the old code used. Unfortunately, we can't directly return the return value on Python because of an old ctypes bug (https://bugs.python.org/issue5710). Instead, input an out param for the return type. The other main possibility would be to change `RustBuffer` to be a simple `*mut u8` (mozilla#1779), which would then be returnable by Python. However, it seems bad to restrict ourselves from ever returning a struct in the future. Eventually, we want to stop using `RustBuffer` for all complex data types and that probably means using a struct instead in some cases. Renamed `CALL_PANIC` to `CALL_UNEXPECTED_ERROR` in the foreign bindings templates. This matches the name in the Rust code and makes more sense for foreign trait implementations. -------------------- TODO ----------------------------- This currently requires "wrapping" the object every time it's passed across the FFI. If the foreign code receives a trait object, then passes it back to Rust. Rust now has a handle to the foreign impl and that foreign impl just calls back into Rust. I think mozilla#1730 could help solve this. I think there should be better tests for reference counts, but I'm waiting until we address the previous issue to implement them. Kotlin is not implemented yet. I think when we do implement it, we're going to want to face mozilla#1394 and make a decision there. I don't think it's reasonable to have consumers have to call a `close()` method on these interfaces. I also don't know how we would deal with this in Rust. ISTM that the same logic that says Kotlin objects need to be manually closed dictates that if you use a Kotlin object in Rust, there should be a `close()` method or something analogous to a to `AutoCloseable`. The Ruby coverall tests are now failing because they don't implement trait interfaces. We need to figure out a way for them to pass the test suite. Remove all the debugging printouts Document this

This adds support for the foreign bindings side to implement Rust traits and for both sides to pass `Arc<dyn [Trait]>` objects back and forth. I thought this would be a good time to redo the callback interface code. The trait interface code uses a foreign-supplied vtable rather than a single callback and method indexes like the old code used. Unfortunately, we can't directly return the return value on Python because of an old ctypes bug (https://bugs.python.org/issue5710). Instead, input an out param for the return type. The other main possibility would be to change `RustBuffer` to be a simple `*mut u8` (mozilla#1779), which would then be returnable by Python. However, it seems bad to restrict ourselves from ever returning a struct in the future. Eventually, we want to stop using `RustBuffer` for all complex data types and that probably means using a struct instead in some cases. Renamed `CALL_PANIC` to `CALL_UNEXPECTED_ERROR` in the foreign bindings templates. This matches the name in the Rust code and makes more sense for foreign trait implementations. -------------------- TODO ----------------------------- This currently requires "wrapping" the object every time it's passed across the FFI. If the foreign code receives a trait object, then passes it back to Rust. Rust now has a handle to the foreign impl and that foreign impl just calls back into Rust. I think mozilla#1730 could help solve this. I think there should be better tests for reference counts, but I'm waiting until we address the previous issue to implement them. Document this

Scaffolding: * Generate a struct that implements the trait using a callback interface callback * Make `try_lift` input a callback interface handle and create one of those structs. * Don't use `try_lift` in the trait interface method scaffolding. `try_lift` expects to lift a callback handle, but scaffolding methods are called with a leaked object pointer. * Removed the unused RustCallStatus param from the callback initialization function Kotlin/Python/Swift: * Refactored the callback interface code so it can also be used for trait interfaces. * Changed the callback interface handle map code so that it doesn't try to re-use the handles. If an object is lowered twice, we now generate two different handles. This is required for trait interfaces, and I think it's also would be the right thing for callback interfaces if they could be passed back into the foreign language from Rust. * Make `lower()` return a callback interface handle. * Added some code to clarify how we generate the protocol and the implementation of that protocol for an object Other: * Trait interfaces are still not supported on Ruby. * Updated the coverall bindings tests to test this. * Updated the traits example, although there's definitely more room for improvement. TODO: I think a better handle solution (mozilla#1730) could help with a few things: * We're currently wrapping the object every time it's passed across the FFI. If the foreign code receives a trait object, then passes it back to Rust. Rust now has a handle to the foreign impl and that foreign impl just calls back into Rust. This can lead to some extremely inefficent FFI calls if an object is passed around enough. * The way we're coercing between pointers, usize, and uint64 is probably wrong and at the very least extremely brittle. There should be better tests for reference counts, but I'm waiting until we address the handle issue to implement them.

Scaffolding: * Generate a struct that implements the trait using a callback interface callback * Make `try_lift` input a callback interface handle and create one of those structs. * Don't use `try_lift` in the trait interface method scaffolding. `try_lift` expects to lift a callback handle, but scaffolding methods are called with a leaked object pointer. * Removed the unused RustCallStatus param from the callback initialization function Kotlin/Python/Swift: * Factored out the callback interface impl and interface/protocol templates so it can also be used for trait interfaces. * Changed the callback interface handle map code so that it doesn't try to re-use the handles. If an object is lowered twice, we now generate two different handles. This is required for trait interfaces, and I think it's also would be the right thing for callback interfaces if they could be passed back into the foreign language from Rust. * Make `lower()` return a callback interface handle. * Added some code to clarify how we generate the protocol and the implementation of that protocol for an object Other: * Trait interfaces are still not supported on Ruby. * Updated the coverall bindings tests to test this. * Updated the traits example, although there's definitely more room for improvement. TODO: I think a better handle solution (mozilla#1730) could help with a few things: * We're currently wrapping the object every time it's passed across the FFI. If the foreign code receives a trait object, then passes it back to Rust. Rust now has a handle to the foreign impl and that foreign impl just calls back into Rust. This can lead to some extremely inefficent FFI calls if an object is passed around enough. * The way we're coercing between pointers, usize, and uint64 is probably wrong and at the very least extremely brittle. There should be better tests for reference counts, but I'm waiting until we address the handle issue to implement them.

This will be used for passing handles across the FFI. Use proptest to test the Rust implementation. The foreign implementations don't have tests, but they are basically carbon copies of the Rust one.

Scaffolding: * Generate a struct that implements the trait using a callback interface callback * Make `try_lift` input a callback interface handle and create one of those structs. * Don't use `try_lift` in the trait interface method scaffolding. `try_lift` expects to lift a callback handle, but scaffolding methods are called with a leaked object pointer. * Removed the unused RustCallStatus param from the callback initialization function Kotlin/Python/Swift: * Factored out the callback interface impl and interface/protocol templates so it can also be used for trait interfaces. * Changed the callback interface handle map code so that it doesn't try to re-use the handles. If an object is lowered twice, we now generate two different handles. This is required for trait interfaces, and I think it's also would be the right thing for callback interfaces if they could be passed back into the foreign language from Rust. * Make `lower()` return a callback interface handle. * Added some code to clarify how we generate the protocol and the implementation of that protocol for an object Other: * Trait interfaces are still not supported on Ruby. * Updated the coverall bindings tests to test this. * Updated the traits example, although there's definitely more room for improvement. TODO: I think a better handle solution (mozilla#1730) could help with a few things: * We're currently wrapping the object every time it's passed across the FFI. If the foreign code receives a trait object, then passes it back to Rust. Rust now has a handle to the foreign impl and that foreign impl just calls back into Rust. This can lead to some extremely inefficent FFI calls if an object is passed around enough. * The way we're coercing between pointers, usize, and uint64 is probably wrong and at the very least extremely brittle. There should be better tests for reference counts, but I'm waiting until we address the handle issue to implement them.

Scaffolding: * Generate a struct that implements the trait using a callback interface callback * Make `try_lift` input a callback interface handle and create one of those structs. * Don't use `try_lift` in the trait interface method scaffolding. `try_lift` expects to lift a callback handle, but scaffolding methods are called with a leaked object pointer. * Removed the unused RustCallStatus param from the callback initialization function Kotlin/Python/Swift: * Factored out the callback interface impl and interface/protocol templates so it can also be used for trait interfaces. * Changed the callback interface handle map code so that it doesn't try to re-use the handles. If an object is lowered twice, we now generate two different handles. This is required for trait interfaces, and I think it's also would be the right thing for callback interfaces if they could be passed back into the foreign language from Rust. * Make `lower()` return a callback interface handle. * Added some code to clarify how we generate the protocol and the implementation of that protocol for an object Other: * Trait interfaces are still not supported on Ruby. * Updated the coverall bindings tests to test this. * Updated the traits example, although there's definitely more room for improvement. TODO: I think a better handle solution (#1730) could help with a few things: * We're currently wrapping the object every time it's passed across the FFI. If the foreign code receives a trait object, then passes it back to Rust. Rust now has a handle to the foreign impl and that foreign impl just calls back into Rust. This can lead to some extremely inefficent FFI calls if an object is passed around enough. * The way we're coercing between pointers, usize, and uint64 is probably wrong and at the very least extremely brittle. There should be better tests for reference counts, but I'm waiting until we address the handle issue to implement them.

This will be used for passing handles across the FFI. Use proptest to test the Rust implementation. Created a new fixture to test foreign bindings code for this. The docs list list of advantages to this, the main disadvantage is performance. We need to allocate a vec entry for the pointer and insert/remove/get need to take a lock. Compared `Arc<ConcreteType>` this is clearly worse. However, compared to `Arc<dyn Trait>` it's not so clear when we currently box the arc before passing it across the FFI. Compared to the foreign HandleMaps, I think the new system will be faster. It seems reasonable to trade some performance for the gains in simplicity and safety. The one thing that really bugs me is that `get()` needs to take a read lock. I'm pretty sure there's a couple ways to fix this, but I'm not sure if it's worth the complexity.

This will be used for passing handles across the FFI. This have several advantages as an FFI type: * Generation counter to detect use-after-free bugs * Slab ID to detect using a handle with the wrong type. * The same data structures can be used on the foreign side, rather than us having to figure out how to leak references in all languages. * Integers come with less gotchas. For example, we use a bit to differentiate between foreign and Rust handles. This would be possible with tagged pointers but there's a lot of details to worry about there. See the `tagged_pointer` crate some. * Our current code mixes actual pointers and usize integers. For example, the callback continuation is a leaked pointer on Swift, but a usize map key on Kotlin. * Constant width at 64 bits rather than using the platform word size. This will simplify some things, especially reading/writing them to `RustBuffer` * Only the first 48 bits are significant which helps with languages like JS. Performance should be pretty good. Insert/get/remove are all lock-free thanks to the `append_only_vec` crate and some atomic code: * For objects, this represents a small overhead over simply leaking the Arc. The same is true for the Swift objects that we leak using `Unmanaged<>`. * For trait interfaces, this is probably a small gain compared to adding an extra box, then leaking it. * This is going to be way faster than the foreign code that uses a lock and a map. The main disadvantage is the extra complexity, but it seems relatively small to me. The stress tests and loom tests give us good confidence that the code is correct. As mentioned above, I'm pretty sure that we can leverage this for foreign handles as well, and should be able to remove some code on from the bindings.

This will be used for passing handles across the FFI. This have several advantages as an FFI type: * Generation counter to detect use-after-free bugs * Slab ID to detect using a handle with the wrong type. * The same data structures can be used on the foreign side, rather than us having to figure out how to leak references in all languages. * Integers come with less gotchas. For example, we use a bit to differentiate between foreign and Rust handles. This would be possible with tagged pointers but there's a lot of details to worry about there. See the `tagged_pointer` crate. * Constant width at 64 bits rather than using the platform word size. This will simplify some things, especially reading/writing them to `RustBuffer` * Only the first 48 bits are significant which helps with languages like JS. Performance should be pretty good. `get` and `inc_ref` are basically lock-free thanks to the `append_only_vec` crate and some atomic code. `insert` and `remove` that frees an entry use a shared lock. We could speed that up by keeping thread-local free lists, but that seems like overkill since the lock should rarely be contended. * For objects, this represents a small overhead over simply leaking the Arc. The same is true for the Swift objects that we leak using `Unmanaged<>`. * For trait interfaces, this is probably a small gain compared to adding an extra box, then leaking it. * This is going to be way faster than the foreign code that uses a lock and a map. As mentioned above, I'm pretty sure that we can leverage this for foreign handles as well, and should be able to remove some code on from the bindings.

bendk mentioned this issue Oct 12, 2023

Foreign trait interfaces #1791

Merged

bendk mentioned this issue Oct 26, 2023

Slabs and handles #1808

Closed

bendk added the FFI-1.0 label Nov 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better system for handles #1730

Better system for handles #1730

bendk commented Sep 5, 2023

Better system for handles #1730

Better system for handles #1730

Comments

bendk commented Sep 5, 2023