Handles

CHANGELOG.md

@@ -18,6 +18,12 @@
 
 - The `rust_future_continuation_callback_set` FFI function was removed.  `rust_future_poll` now
   inputs the callback pointer.  External bindings authors will need to update their code.
+- The object handle FFI has changed.  External bindings generators will need to update their code to
+  use the new handle system:
+    * A single `FfiType::Handle` is used for all object handles.
+    * `FfiType::Handle` is always a 64-bit int.
+    * Foreign handles must always set the lowest bit of that int.
+- The `NoPointer` singleton was renamed to `NoHandle`
 
 ### What's new?
 

fixtures/coverall/tests/bindings/test_coverall.kts

@@ -446,11 +446,11 @@ Coveralls("test_bytes").use { coveralls ->
 
 // Test fakes using open classes
 
-class FakePatch(private val color: Color): Patch(NoPointer) {
+class FakePatch(private val color: Color): Patch(NoHandle) {
     override fun `getColor`(): Color = color
 }
 
-class FakeCoveralls(private val name: String) : Coveralls(NoPointer) {
+class FakeCoveralls(private val name: String) : Coveralls(NoHandle) {
     private val repairs = mutableListOf<Repair>()
 
     override fun `addPatch`(patch: Patch) {

fixtures/foreign-executor/tests/bindings/test_foreign_executor.kts

@@ -37,11 +37,3 @@ runBlocking {
     assert(result.delayMs <= 200U)
     tester.close()
 }
-
-// Test that we cleanup when dropping a ForeignExecutor handles
-assert(FfiConverterForeignExecutor.handleCount() == 0)
-val tester = ForeignExecutorTester(coroutineScope)
-val tester2 = ForeignExecutorTester.newFromSequence(listOf(coroutineScope))
-tester.close()
-tester2.close()
-assert(FfiConverterForeignExecutor.handleCount() == 0)

fixtures/uitests/tests/ui/interface_trait_not_sync_and_send.stderr

@@ -1,3 +1,31 @@
+error[E0277]: `(dyn Trait + 'static)` cannot be shared between threads safely
+ --> $OUT_DIR[uniffi_uitests]/trait.uniffi.rs
+  |
+  | #[::uniffi::export_for_udl]
+  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ `(dyn Trait + 'static)` cannot be shared between threads safely
+  |
+  = help: the trait `Sync` is not implemented for `(dyn Trait + 'static)`
+note: required by a bound in `HandleAlloc`
+ --> $WORKSPACE/uniffi_core/src/ffi_converter_traits.rs
+  |
+  | pub unsafe trait HandleAlloc<UT>: Send + Sync {
+  |                                          ^^^^ required by this bound in `HandleAlloc`
+  = note: this error originates in the attribute macro `::uniffi::export_for_udl` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error[E0277]: `(dyn Trait + 'static)` cannot be sent between threads safely
+ --> $OUT_DIR[uniffi_uitests]/trait.uniffi.rs
+  |
+  | #[::uniffi::export_for_udl]
+  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ `(dyn Trait + 'static)` cannot be sent between threads safely
+  |
+  = help: the trait `Send` is not implemented for `(dyn Trait + 'static)`
+note: required by a bound in `HandleAlloc`
+ --> $WORKSPACE/uniffi_core/src/ffi_converter_traits.rs
+  |
+  | pub unsafe trait HandleAlloc<UT>: Send + Sync {
+  |                                   ^^^^ required by this bound in `HandleAlloc`
+  = note: this error originates in the attribute macro `::uniffi::export_for_udl` (in Nightly builds, run with -Z macro-backtrace for more info)
+
 error[E0277]: `(dyn Trait + 'static)` cannot be shared between threads safely
  --> $OUT_DIR[uniffi_uitests]/trait.uniffi.rs
   |
@@ -26,6 +54,34 @@ note: required by a bound in `FfiConverterArc`
   |                                       ^^^^ required by this bound in `FfiConverterArc`
   = note: this error originates in the attribute macro `::uniffi::export_for_udl` (in Nightly builds, run with -Z macro-backtrace for more info)
 
+error[E0277]: `(dyn ProcMacroTrait + 'static)` cannot be shared between threads safely
+  --> tests/ui/interface_trait_not_sync_and_send.rs:11:1
+   |
+11 | #[uniffi::export]
+   | ^^^^^^^^^^^^^^^^^ `(dyn ProcMacroTrait + 'static)` cannot be shared between threads safely
+   |
+   = help: the trait `Sync` is not implemented for `(dyn ProcMacroTrait + 'static)`
+note: required by a bound in `HandleAlloc`
+  --> $WORKSPACE/uniffi_core/src/ffi_converter_traits.rs
+   |
+   | pub unsafe trait HandleAlloc<UT>: Send + Sync {
+   |                                          ^^^^ required by this bound in `HandleAlloc`
+   = note: this error originates in the attribute macro `uniffi::export` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error[E0277]: `(dyn ProcMacroTrait + 'static)` cannot be sent between threads safely
+  --> tests/ui/interface_trait_not_sync_and_send.rs:11:1
+   |
+11 | #[uniffi::export]
+   | ^^^^^^^^^^^^^^^^^ `(dyn ProcMacroTrait + 'static)` cannot be sent between threads safely
+   |
+   = help: the trait `Send` is not implemented for `(dyn ProcMacroTrait + 'static)`
+note: required by a bound in `HandleAlloc`
+  --> $WORKSPACE/uniffi_core/src/ffi_converter_traits.rs
+   |
+   | pub unsafe trait HandleAlloc<UT>: Send + Sync {
+   |                                   ^^^^ required by this bound in `HandleAlloc`
+   = note: this error originates in the attribute macro `uniffi::export` (in Nightly builds, run with -Z macro-backtrace for more info)
+
 error[E0277]: `(dyn ProcMacroTrait + 'static)` cannot be shared between threads safely
   --> tests/ui/interface_trait_not_sync_and_send.rs:11:1
    |

uniffi_bindgen/src/bindings/kotlin/gen_kotlin/mod.rs

@@ -327,19 +327,16 @@ impl KotlinCodeOracle {
             FfiType::Int64 | FfiType::UInt64 => "Long".to_string(),
             FfiType::Float32 => "Float".to_string(),
             FfiType::Float64 => "Double".to_string(),
-            FfiType::RustArcPtr(_) => "Pointer".to_string(),
+            FfiType::Handle => "UniffiHandle".to_string(),
             FfiType::RustBuffer(maybe_suffix) => {
                 format!("RustBuffer{}", maybe_suffix.as_deref().unwrap_or_default())
             }
             FfiType::ForeignBytes => "ForeignBytes.ByValue".to_string(),
             FfiType::ForeignCallback => "ForeignCallback".to_string(),
-            FfiType::ForeignExecutorHandle => "USize".to_string(),
             FfiType::ForeignExecutorCallback => "UniFfiForeignExecutorCallback".to_string(),
-            FfiType::RustFutureHandle => "Pointer".to_string(),
             FfiType::RustFutureContinuationCallback => {
                 "UniFffiRustFutureContinuationCallbackType".to_string()
             }
-            FfiType::RustFutureContinuationData => "USize".to_string(),
         }
     }
 

uniffi_bindgen/src/bindings/kotlin/templates/Async.kt

@@ -3,20 +3,20 @@
 internal const val UNIFFI_RUST_FUTURE_POLL_READY = 0.toShort()
 internal const val UNIFFI_RUST_FUTURE_POLL_MAYBE_READY = 1.toShort()
 
-internal val uniffiContinuationHandleMap = UniFfiHandleMap<CancellableContinuation<Short>>()
+internal val uniffiContinuationHandleMap = UniffiHandleMap<CancellableContinuation<Short>>()
 
 // FFI type for Rust future continuations
 internal object uniffiRustFutureContinuationCallback: UniFffiRustFutureContinuationCallbackType {
-    override fun callback(continuationHandle: USize, pollResult: Short) {
-        uniffiContinuationHandleMap.remove(continuationHandle)?.resume(pollResult)
+    override fun callback(continuationHandle: UniffiHandle, pollResult: Short) {
+        uniffiContinuationHandleMap.consumeHandle(continuationHandle).resume(pollResult)
     }
 }
 
 internal suspend fun<T, F, E: Exception> uniffiRustCallAsync(
-    rustFuture: Pointer,
-    pollFunc: (Pointer, UniFffiRustFutureContinuationCallbackType, USize) -> Unit,
-    completeFunc: (Pointer, RustCallStatus) -> F,
-    freeFunc: (Pointer) -> Unit,
+    rustFuture: UniffiHandle,
+    pollFunc: (UniffiHandle, UniFffiRustFutureContinuationCallbackType, UniffiHandle) -> Unit,
+    completeFunc: (UniffiHandle, RustCallStatus) -> F,
+    freeFunc: (UniffiHandle) -> Unit,
     liftFunc: (F) -> T,
     errorHandler: CallStatusErrorHandler<E>
 ): T {
@@ -26,7 +26,7 @@ internal suspend fun<T, F, E: Exception> uniffiRustCallAsync(
                 pollFunc(
                     rustFuture,
                     uniffiRustFutureContinuationCallback,
-                    uniffiContinuationHandleMap.insert(continuation)
+                    uniffiContinuationHandleMap.newHandle(continuation)
                 )
             }
         } while (pollResult != UNIFFI_RUST_FUTURE_POLL_READY);

uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceImpl.kt

@@ -3,11 +3,11 @@
 // Implement the foreign callback handler for {{ interface_name }}
 internal class {{ callback_handler_class }} : ForeignCallback {
     @Suppress("TooGenericExceptionCaught")
-    override fun invoke(handle: Handle, method: Int, argsData: Pointer, argsLen: Int, outBuf: RustBufferByReference): Int {
+    override fun invoke(handle: UniffiHandle, method: Int, argsData: Pointer, argsLen: Int, outBuf: RustBufferByReference): Int {
         val cb = {{ ffi_converter_name }}.handleMap.get(handle)
         return when (method) {
             IDX_CALLBACK_FREE -> {
-                {{ ffi_converter_name }}.handleMap.remove(handle)
+                {{ ffi_converter_name }}.handleMap.consumeHandle(handle)
 
                 // Successful return
                 // See docs of ForeignCallback in `uniffi_core/src/ffi/foreigncallbacks.rs`

uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceRuntime.kt

@@ -1,39 +1,8 @@
 {{- self.add_import("java.util.concurrent.atomic.AtomicLong") }}
 {{- self.add_import("java.util.concurrent.locks.ReentrantLock") }}
-{{- self.add_import("kotlin.concurrent.withLock") }}
-
-internal typealias Handle = Long
-internal class ConcurrentHandleMap<T>(
-    private val leftMap: MutableMap<Handle, T> = mutableMapOf(),
-) {
-    private val lock = java.util.concurrent.locks.ReentrantLock()
-    private val currentHandle = AtomicLong(0L)
-    private val stride = 1L
-
-    fun insert(obj: T): Handle =
-        lock.withLock {
-            currentHandle.getAndAdd(stride)
-                .also { handle ->
-                    leftMap[handle] = obj
-                }
-            }
-
-    fun get(handle: Handle) = lock.withLock {
-        leftMap[handle] ?: throw InternalException("No callback in handlemap; this is a Uniffi bug")
-    }
-
-    fun delete(handle: Handle) {
-        this.remove(handle)
-    }
-
-    fun remove(handle: Handle): T? =
-        lock.withLock {
-            leftMap.remove(handle)
-        }
-}
 
 interface ForeignCallback : com.sun.jna.Callback {
-    public fun invoke(handle: Handle, method: Int, argsData: Pointer, argsLen: Int, outBuf: RustBufferByReference): Int
+    public fun invoke(handle: UniffiHandle, method: Int, argsData: Pointer, argsLen: Int, outBuf: RustBufferByReference): Int
 }
 
 // Magic number for the Rust proxy to call using the same mechanism as every other method,
@@ -44,20 +13,16 @@ internal const val UNIFFI_CALLBACK_SUCCESS = 0
 internal const val UNIFFI_CALLBACK_ERROR = 1
 internal const val UNIFFI_CALLBACK_UNEXPECTED_ERROR = 2
 
-public abstract class FfiConverterCallbackInterface<CallbackInterface>: FfiConverter<CallbackInterface, Handle> {
-    internal val handleMap = ConcurrentHandleMap<CallbackInterface>()
-
-    internal fun drop(handle: Handle) {
-        handleMap.remove(handle)
-    }
+public abstract class FfiConverterCallbackInterface<CallbackInterface>: FfiConverter<CallbackInterface, UniffiHandle> {
+    internal val handleMap = UniffiHandleMap<CallbackInterface>()
 
-    override fun lift(value: Handle): CallbackInterface {
+    override fun lift(value: UniffiHandle): CallbackInterface {
         return handleMap.get(value)
     }
 
     override fun read(buf: ByteBuffer) = lift(buf.getLong())
 
-    override fun lower(value: CallbackInterface) = handleMap.insert(value)
+    override fun lower(value: CallbackInterface) = handleMap.newHandle(value)
 
     override fun allocationSize(value: CallbackInterface) = 8
 

uniffi_bindgen/src/bindings/kotlin/templates/ForeignExecutorTemplate.kt

@@ -16,7 +16,7 @@ internal interface UniFfiRustTaskCallback : com.sun.jna.Callback {
 }
 
 internal object UniFfiForeignExecutorCallback : com.sun.jna.Callback {
-    fun callback(handle: USize, delayMs: Int, rustTask: UniFfiRustTaskCallback?, rustTaskData: Pointer?) : Byte {
+    fun callback(handle: UniffiHandle, delayMs: Int, rustTask: UniFfiRustTaskCallback?, rustTaskData: Pointer?) : Byte {
         if (rustTask == null) {
             FfiConverterForeignExecutor.drop(handle)
             return UNIFFI_FOREIGN_EXECUTOR_CALLBACK_SUCCESS
@@ -42,11 +42,11 @@ internal object UniFfiForeignExecutorCallback : com.sun.jna.Callback {
     }
 }
 
-public object FfiConverterForeignExecutor: FfiConverter<CoroutineScope, USize> {
-    internal val handleMap = UniFfiHandleMap<CoroutineScope>()
+public object FfiConverterForeignExecutor: FfiConverter<CoroutineScope, UniffiHandle> {
+    internal val handleMap = UniffiHandleMap<CoroutineScope>()
 
-    internal fun drop(handle: USize) {
-        handleMap.remove(handle)
+    internal fun drop(handle: UniffiHandle) {
+        handleMap.consumeHandle(handle)
     }
 
     internal fun register(lib: _UniFFILib) {
@@ -58,26 +58,21 @@ public object FfiConverterForeignExecutor: FfiConverter<CoroutineScope, USize> {
         {% endmatch %}
     }
 
-    // Number of live handles, exposed so we can test the memory management
-    public fun handleCount() : Int {
-        return handleMap.size
-    }
-
-    override fun allocationSize(value: CoroutineScope) = USize.size
+    override fun allocationSize(value: CoroutineScope) = 8
 
-    override fun lift(value: USize): CoroutineScope {
-        return handleMap.get(value) ?: throw RuntimeException("unknown handle in FfiConverterForeignExecutor.lift")
+    override fun lift(value: UniffiHandle): CoroutineScope {
+        return handleMap.get(value)
     }
 
     override fun read(buf: ByteBuffer): CoroutineScope {
-        return lift(USize.readFromBuffer(buf))
+        return lift(buf.getLong())
     }
 
-    override fun lower(value: CoroutineScope): USize {
-        return handleMap.insert(value)
+    override fun lower(value: CoroutineScope): UniffiHandle {
+        return handleMap.newHandle(value)
     }
 
     override fun write(value: CoroutineScope, buf: ByteBuffer) {
-        lower(value).writeToBuffer(buf)
+        buf.putLong(lower(value))
     }
 }

uniffi_bindgen/src/bindings/kotlin/templates/HandleMap.kt

@@ -0,0 +1,53 @@
+internal class UniffiHandleMap<T> {
+    private val lock = ReentrantReadWriteLock()
+    private var mapId: Long = UniffiHandleMap.nextMapId()
+    private val map: MutableMap<Long, T> = mutableMapOf()
+    // Note: Foreign handles are always odd
+    private var keyCounter = 1L
+
+    private fun nextKey(): Long = keyCounter.also {
+        keyCounter = (keyCounter + 2L).and(0xFFFF_FFFF_FFFFL)
+    }
+
+    private fun makeHandle(key: Long): UniffiHandle = key.or(mapId)
+
+    private fun key(handle: UniffiHandle): Long {
+        if (handle.and(0x7FFF_0000_0000_0000L) != mapId) {
+            throw InternalException("Handle map ID mismatch")
+        }
+        return handle.and(0xFFFF_FFFF_FFFFL)
+    }
+
+    fun newHandle(obj: T): UniffiHandle = lock.writeLock().withLock {
+        val key = nextKey()
+        map[key] = obj
+        makeHandle(key)
+    }
+
+    fun get(handle: UniffiHandle) = lock.readLock().withLock {
+        map[key(handle)] ?: throw InternalException("Missing key in handlemap: was the handle used after being freed?")
+    }
+
+    fun cloneHandle(handle: UniffiHandle): UniffiHandle = lock.writeLock().withLock {
+        val obj = map[key(handle)] ?: throw InternalException("Missing key in handlemap: was the handle used after being freed?")
+        val clone = nextKey()
+        map[clone] = obj
+        makeHandle(clone)
+    }
+
+    fun consumeHandle(handle: UniffiHandle): T = lock.writeLock().withLock {
+        map.remove(key(handle)) ?: throw InternalException("Missing key in handlemap: was the handle used after being freed?")
+    }
+
+    companion object {
+        // Generate map IDs that are likely to be unique
+        private var mapIdCounter: Long = {{ ci.namespace_hash() }}.and(0x7FFF)
+
+        // Map ID, shifted into the top 16 bits
+        internal fun nextMapId(): Long = mapIdCounter.shl(48).also {
+            // On Kotlin, map ids are only 15 bits to get around signed/unsigned issues
+            mapIdCounter = ((mapIdCounter + 1).and(0x7FFF))
+        }
+    }
+}
+