Add callback interfaces for Kotlin #344

jhugman · 2020-11-06T22:33:31Z

This PR implements the WebIDL callback interface, to setup Rust calling into Kotlin.

A callback interface describes a trait in Rust. Uniffi generates a corresponding interface in Kotlin for the client code to implement. The client code sends the implementation to rust, where Rust treats it as a Box<dyn Trait>. Any call to that trait is then sent back to Kotlin.

Related to #342 . (fixes for Kotlin)
Fixes #352 .

Example Problem

We'd like to have a Rust trait to fetch a document which will drive experiments:

trait FetchClient {
    fn fetch(filepath: String) -> String
}

struct NimbusClient {
    client: Box<dyn FetchClient>,
}

impl NimbusClient {
    fn set_fetch_client(client: Box<dyn FetchClient>) {
        self.client = client;
    }

   fn update_experiments() {
        let json_string =  self.client.fetch("experiments.json");
        …
    }
}

Declared in the IDL:

callback interface FetchClient {
    string fetch(string filepath);
};

interface NimbusClient {
    set_fetch_client(FetchClient client);
    update_experiments();
};

And used in Kotlin:

class ConceptFetchClient(
    val baseURL: String
): FetchClient {
    override fun fetch(filepath: String): String {
       let url = URL(baseURL, filepath)
       …
    }
}

val nimbus = NimbusClient()
nimbus.setFetchClient(ConceptFetchClient(BASE_URL))
nimbus.updateExperiments() // uses ConceptFetchClient

Implementation

This PR presents a strawfox implementation.

We sends JNA Callbacks to send a callback interface to Rust (a.k.a. ForeignCallback).

This implementation of the ForeignCallback is individual to the callback interface— on the kotlin side; in this case this is called: CallbackInterfaceFetchClientFFI.

As the ConceptFetchClient is sent from kotlin, it is placed in a kotlin ConcurrentHandleMap.

A Rust impl of corresponding FetchClient trait is generated which proxies all calls through the ForeignCallback to kotlin, i.e. CallbackInterfaceFetchClientProxy in Rust calls to CallbackInterfaceFetchClientFFI in kotlin.

The callback object i.e. the ConceptFetchClient is then found, and then methods are called from CallbackInterfaceFetchClientFFI to the client code ConceptFetchClient.

Issues

No support for errors, or Result.
I'm a bit worried about threading with a second concurrent handlemap and the callback being able to call back into Rust.
I'm a bit worried about deadlocks in the same way as Support returning objects from functions and methods #197 (we're passing callbacks across the FFI boundary)
Testing of the Kotlin handlemap is non-existent; I have been unable to test if Drop was working properly with a WeakReference, but that should be more easily tested with handlemap testing.
I've cargo culted quite a lot of the viaduct code for setting callbacks, though I don't know if this is still the best way of doing things.
Other rusty things to do with ffi_support and saftey, I'm not super confident on.

I'm not sure about implementing Send, but it seemed the only way I could get a callback interface storable in an object.

rfk · 2020-11-10T05:15:37Z

Sorry I haven't had time to look at this in any detail, and I probably won't this week either :-(
Ping me if I don't seem to have gotten around to it by next week?

jhugman

First pass with comments.

jhugman · 2020-11-07T13:22:46Z

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

+    fun <R> callWithResult(handle: Handle, fn: (T) -> R): R =
+        lock.withLock {
+            val obj = leftMap[handle] ?: throw RuntimeException("Panic: handle not in handlemap")
+            fn.invoke(obj)
+        }


I'm nervous that invoking fn while still within the critical section could lead to deadlock.

Suggested change

fun <R> callWithResult(handle: Handle, fn: (T) -> R): R =

lock.withLock {

val obj = leftMap[handle] ?: throw RuntimeException("Panic: handle not in handlemap")

fn.invoke(obj)

}

fun <R> callWithResult(handle: Handle, fn: (T) -> R): R =

lock.withLock { leftMap[handle] ?: throw RuntimeException("Panic: handle not in handlemap") }

.let { fn.invoke(it) }

Right, good call. And things should be safe here once we've got a reference to the object out of the handlemap, because the GC will keep it alive during the call.

The ConcurrentHandleMap in Rust sounds like it has this same implementation and similar deadlock weakness.

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

rfk

This is impressive stuff! I left a bunch of comments as I worked my way through it, but I don't think I came away with any substantial concerns.

@jhugman are you looking to merge this and iterate, or are you after an initial round of feedback and plan to keep working on some of the outstanding things mentioned in the PR description?

The one thing I do think we need is some narrative docs about the high-level design of the system (like the kind of docs we wish we had about the serialization format). What do you think about starting a "design docs" subdirectory under ./docs to contain such a thing?

examples/callbacks/src/callbacks.udl

rfk · 2020-11-17T04:33:46Z

examples/callbacks/tests/bindings/test_callbacks.kts

+    val observed = st.fromSimpleType(v)
+    assert(expected == observed) { "callback is sent on construction: $expected != $observed" }
+}
+st.destroy()


I haven't dug into the implementation yet, but I reached this point in reading the PR and the mechanics of callbacks seem to make sense to me as a user. So, so far so good 👍

uniffi/src/ffi/foreigncallbacks.rs

rfk · 2020-11-17T05:28:56Z

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

+                {% let method_name = format!("invoke_{}", meth.name())|fn_name_kt -%}
+                {{ loop.index }} -> this.{{ method_name }}(cb, args)
+                {% endfor %}
+                else -> RustBuffer.ByValue()


In some future iteration, I guess this is where we'd signal an error back to the Rust code. Any thoughts on how that might work? (Or should we just forbid it for now?)

I haven't spent too long working out how to pass back or signal errors.

The way viaduct does it is to pass back a string through to rust and then deserialize into a Response.

At a scrappy minimum, we should probably use the same sort of error syntax for interfaces, and log the stacktrace on the Kotlin side.

However we implement it, I guess we should probably use the same sort of error syntax for interfaces.

Broadly agreed, the more symmetry we can have between interface and callback interface the better things are likely to be.

I opened up #351 to track this.

Could you please add a brief TODO comment in the code here with a link to the follow-up issue? I think it's important that we remind our future selves that the else branch here is a temporary situation.

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

rfk · 2020-11-17T05:38:53Z

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

+    fun <R> callWithResult(handle: Handle, fn: (T) -> R): R =
+        lock.withLock {
+            val obj = leftMap[handle] ?: throw RuntimeException("Panic: handle not in handlemap")
+            fn.invoke(obj)
+        }


Right, good call. And things should be safe here once we've got a reference to the object out of the handlemap, because the GC will keep it alive during the call.

rfk · 2020-11-17T05:40:51Z

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

+
+    fun lift(n: Long) = handleMap.get(n)
+
+    fun read(buf: ByteBuffer) = lift(buf.getLong())


As with the Rust side of things, I'm kind of dubious about lift/read/write here, since I don't think we can guarantee that the handle is still good.

I don't think we can guarantee that the handle is still good.

I'm not sure I understand why it's not true. I'll have to think about this.

The only way we get a stale handle is if the handle is removed from the handlemap.

The only way to remove it from the handlemap is to call drop.

The only way client code calls drop is in the Rust trait Drop.

I'm less confident in my reasoning because the Rust compiler wants us to implement Send.

I don't think this (removing lift/read/write) should stop us from landing the PR however.

Revisiting this, I'm not sure I recall exactly what my concern was. I guess the other implicit link in the chain of argument here is that we don't ever read or write as part of some long-term serialization that might keep a reference to the handle around after it's been freed, we only do them as part of a transient shuffling of data from one side of the FFI to the other. So...I think I'm OK with this having concrete read and write implementations 👍

rfk · 2020-11-17T05:49:22Z

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

+        {#- Unpacking args from the RustBuffer #}
+            {%- if meth.arguments().len() != 0 -%}
+            {#- Calling the concrete callback object #}
+            val buf = args.asByteBuffer() ?: throw InternalError("No ByteBuffer in RustBuffer; this is a Uniffi bug")


What will actually happen to this exception if it does get thrown? Or equivalently, what will happen if the user's provided implementation method throws?

The JNA docs say:

A callback should generally never throw an exception, since it doesn't necessarily have an encompassing
Java environment to catch it. Any exceptions thrown will be passed to the default callback exception handler.

But it's not at all obvious to me what that actually means, or how the rust code will respond if allowed to continue running after this happens.

Should we try to catch exceptions and signal the error to the Rust code, maybe using an out ExternError argument like we do for calls from the foreign language into Rust?

Yes, agreed. Catching the exception is the easy bit, what to do with it after is hard.

We've got [Error] and [Throws=] annotations; I don't understand ExternError enough to make a judgement here.

jhugman · 2020-11-17T15:29:01Z

are you looking to merge this and iterate, or are you after an initial round of feedback and plan to keep working on some of the outstanding things mentioned in the PR description?

AFAICT, the biggest unresolved issue left is trapping, logging and returning errors, which I'm not sure of the best path forward here.

I think a round of feedback would be useful and then spin out the missing pieces in to separate issues. i.e. land and iterate.

However, this isn't yet on the critical path, so it's going to be a while before it's ready to land.

travis79

This is awesome! I defer to @rfk's expertise on this, but I'm excited to try it out.

jhugman

@rfk Thanks for feedback! I think this is ready for review now.

jhugman · 2020-11-30T18:53:49Z

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

+                {% let method_name = format!("invoke_{}", meth.name())|fn_name_kt -%}
+                {{ loop.index }} -> this.{{ method_name }}(cb, args)
+                {% endfor %}
+                else -> RustBuffer.ByValue()


I opened up #351 to track this.

jhugman · 2020-11-30T18:58:45Z

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

+        {#- Unpacking args from the RustBuffer #}
+            {%- if meth.arguments().len() != 0 -%}
+            {#- Calling the concrete callback object #}
+            val buf = args.asByteBuffer() ?: throw InternalError("No ByteBuffer in RustBuffer; this is a Uniffi bug")


Yes, agreed. Catching the exception is the easy bit, what to do with it after is hard.

We've got [Error] and [Throws=] annotations; I don't understand ExternError enough to make a judgement here.

jhugman · 2020-11-30T19:06:13Z

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

+
+    fun lift(n: Long) = handleMap.get(n)
+
+    fun read(buf: ByteBuffer) = lift(buf.getLong())


The only way we get a stale handle is if the handle is removed from the handlemap.

The only way to remove it from the handlemap is to call drop.

The only way client code calls drop is in the Rust trait Drop.

I'm less confident in my reasoning because the Rust compiler wants us to implement Send.

I don't think this (removing lift/read/write) should stop us from landing the PR however.

jhugman · 2020-12-01T13:41:51Z

uniffi_bindgen/src/templates/CallbackInterfaceTemplate.rs

+// i.e. in the handle map.
+// Prepared for our target trait to declare:
+//  `trait {{ trait_name }}: Send + std::fmt::Debug`
+unsafe impl Send for {{ trait_impl }} {}


In short, I don't know it's safe to implement this. I suspect the reason Send isn't derived is because usage of pointers in #L44.

From the nomicon:

However raw pointers are, strictly speaking, marked as thread-unsafe as more of a lint. Doing anything useful with a raw pointer requires dereferencing it, which is already unsafe. In that sense, one could argue that it would be "fine" for them to be marked as thread safe.
However it's important that they aren't thread-safe to prevent types that contain them from being automatically marked as thread-safe. These types have non-trivial untracked ownership, and it's unlikely that their author was necessarily thinking hard about thread safety.

rfk · 2020-12-09T03:41:01Z

Great! I'll make sure this is in my review queue but probably won't get a chance to look at it until end of week or early next.

rfk

Sorry for the review delay here, the end of year kind of got away from me a bit.

This looks great! I left a bunch of small comments and nits and questions but nothing that should block landing this PR and iterating further in followups. I look forward to finding some time to work on a callback implementation for Python based on this :-)

One thing to consider is a brief "high-level overview of how callbacks work" document for our future reference. The module-level docstring of foreigncallbacks.rs might be a good place for this, absent a dedicated "design docs" area in the repo or manual.

rfk · 2020-12-27T00:42:30Z

examples/callbacks/src/callbacks.udl

+/// These objects are implemented by the foreign language and passed 
+/// to Rust. Rust then calls methods on it when it needs to.
+/// Rust developers need to declare these traits extending `Send` so 
+/// They can be stored in Rust— i.e. not passed in as an argument to


nit: capitalization of they.

rfk · 2020-12-27T00:43:55Z

examples/callbacks/src/callbacks.udl

+callback interface StoredForeignStringifier {
+  string from_simple_type(i32 value);
+  // Test if types are collected from callback interfaces.
+  // kotlinc compile time error if not.


to make sure I understand: we're testing whether the f64 here correctly ends up in the TypeUniverse of the resulting ComponentInterface?

rfk · 2020-12-27T00:45:19Z

examples/callbacks/src/lib.rs

+            let _ = call_responder.hello();
+        } else {
+            call_responder.busy();
+            call_responder.text_received("Pas maintenant!".into());


FWIW, I wouldn't be upset if we reverted to English-language example strings outside of rondpoint :-)

rfk · 2020-12-27T00:50:21Z

examples/callbacks/tests/bindings/test_callbacks.kts

+    override fun fromSimpleType(value: Int): String = "kotlin: $value"
+    // We don't test this, but we're checking that the arg type is included in the minimal list of types used
+    // in the UDL.
+    // If this doesn't compile, then look at TypeResolver.


Not for this PR, but the comments here are a good reminder that we should have more thorough tests for this at the Rust level. There's nothing Kotlin-specific about the behaviour of TypeResolver in this regard, we should be able to write a pure Rust test that parses some UDL and then asserts things about the state of the resulting ComponentInterface and its TypeUniverse.

uniffi/src/ffi/foreigncallbacks.rs

rfk · 2020-12-27T01:02:52Z

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

+                {% let method_name = format!("invoke_{}", meth.name())|fn_name_kt -%}
+                {{ loop.index }} -> this.{{ method_name }}(cb, args)
+                {% endfor %}
+                else -> RustBuffer.ByValue()


Could you please add a brief TODO comment in the code here with a link to the follow-up issue? I think it's important that we remind our future selves that the else branch here is a temporary situation.

rfk · 2020-12-27T01:09:03Z

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

+    private val leftMap: MutableMap<Handle, T> = mutableMapOf(),
+    private val rightMap: MutableMap<T, Handle> = mutableMapOf()
+) {
+    private val lock = java.util.concurrent.locks.ReentrantLock()


IIUC, this lock only protects concurrent access to the maps, and we release it before invoking the callback. So, it's possible to have multiple callbacks being dispatched at the same time. Accurate?

Yes, correct.

rfk · 2020-12-27T01:14:12Z

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

+
+    fun lift(n: Long) = handleMap.get(n)
+
+    fun read(buf: ByteBuffer) = lift(buf.getLong())


Revisiting this, I'm not sure I recall exactly what my concern was. I guess the other implicit link in the chain of argument here is that we don't ever read or write as part of some long-term serialization that might keep a reference to the handle around after it's been freed, we only do them as part of a transient shuffling of data from one side of the FFI to the other. So...I think I'm OK with this having concrete read and write implementations 👍

rfk · 2020-12-27T01:16:10Z

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

@@ -57,4 +60,9 @@ import java.util.concurrent.atomic.AtomicLong
 {% include "ObjectTemplate.kt" %}
 {% endfor %}

+// Callback Interfaces
+{% for obj in ci.iter_callback_interface_definitions() %}


nano-nit: consider using a different name for the loop index variable here, e.g. cb or callback or something.

rfk · 2020-12-27T01:36:07Z

uniffi_bindgen/src/templates/CallbackInterfaceTemplate.rs

+// i.e. in the handle map.
+// Prepared for our target trait to declare:
+//  `trait {{ trait_name }}: Send + std::fmt::Debug`
+unsafe impl Send for {{ trait_impl }} {}


I tried commenting out this unsafe impl Send and cargo test still ran successfully to completion, which suggests to me that Rust is now automatically deriving a Send impl for us. Maybe a refactor has made it automatically Send?

What do you think about switching from unsafe impl Send to statically asserting that it is implemented? This would help remind us to re-check any assumptions here if we ever change the struct in a way that makes it no longer automatically Send.

(I'd also support taking a depending on the static_assertions crate for this, as it may be a bit awkward to express this as an expression suitable for use with ffi_support::static_assert!).

We'd like to have a Rust trait: ```rust trait FetchClient { fn fetch(url: String) -> String } // later impl Nimbus { fn set_fetch_client(client: Box<dyn FetchClient>) {} } ``` Declared in the IDL: ```idl interface Nimbus { set_fetch_client(FetchClient client); update_experiments(); } callback interface FetchClient { string fetch(string url); }; ``` And used in Kotlin: ```kotlin class ConceptFetchClient: FetchClient { override fun fetch(url: String): String { ... } } nimbus.setFetchClient(ConceptFetchClient()) nimbus.updateExperiments() // uses ConceptFetchClient ``` This commit sends JNA Callbacks to send a callback interface to Rust (a.k.a. `ForeignCallback`). This implementation of the `ForeignCallback` is individual to the callback interface— on the kotlin side; in this case this is called: `CallbackInterfaceFetchClientFFI`. As the `ConceptFetchClient` is sent from kotlin, it is placed in a kotlin `ConcurrentHandleMap`. A Rust `impl` of corresponding `FetchClient` trait is generated which proxies all calls through the `ForeignCallback` to kotlin, i.e. `CallbackInterfaceFetchClientProxy` in Rust calls to `CallbackInterfaceFetchClientFFI` in kotlin. The callback object i.e. the `ConceptFetchClient` is then found, and then methods are called from `CallbackInterfaceFetchClientFFI` to the client code `ConceptFetchClient`.

jhugman requested review from rfk and eoger November 6, 2020 22:46

jhugman commented Nov 10, 2020

View reviewed changes

jhugman force-pushed the callback-interfaces branch 2 times, most recently from ea97dc1 to 36235b3 Compare November 10, 2020 12:42

jhugman requested a review from travis79 November 10, 2020 17:58

rfk reviewed Nov 17, 2020

View reviewed changes

travis79 reviewed Nov 17, 2020

View reviewed changes

jhugman force-pushed the callback-interfaces branch from 684198f to 5ee7b5a Compare November 30, 2020 18:46

jhugman commented Dec 4, 2020

View reviewed changes

jhugman force-pushed the callback-interfaces branch from 5ee7b5a to 67727a1 Compare December 4, 2020 17:10

rfk approved these changes Dec 27, 2020

View reviewed changes

jhugman force-pushed the callback-interfaces branch from 67727a1 to 9d8f7bb Compare January 14, 2021 19:08

jhugman added 5 commits January 21, 2021 17:02

Address first pass review comments

3212468

Address reviewer comments

90e4743

Review 3 comments

3f965b8

Final review comments

12539d0

jhugman force-pushed the callback-interfaces branch from 9d8f7bb to 12539d0 Compare January 21, 2021 17:04

jhugman merged commit ac5707d into mozilla:main Jan 22, 2021

jhugman deleted the callback-interfaces branch January 22, 2021 12:31

jhugman mentioned this pull request Mar 29, 2021

Implement callback interfaces for Swift #353

Closed

data-sync-user mentioned this pull request Jun 23, 2021

Implement callback interfaces for Swift mozilla/application-services#4284

Closed

This was referenced Oct 4, 2021

Add callback interfaces for Swift #1066

Merged

Add callback interfaces for Python #1068

Closed


		fun lift(n: Long) = handleMap.get(n)

		fun read(buf: ByteBuffer) = lift(buf.getLong())

Add callback interfaces for Kotlin #344

Add callback interfaces for Kotlin #344

Conversation

jhugman commented Nov 6, 2020 • edited Loading

Example Problem

Implementation

Issues

rfk commented Nov 10, 2020

jhugman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhugman commented Nov 17, 2020

travis79 left a comment

Choose a reason for hiding this comment

jhugman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfk commented Dec 9, 2020

rfk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhugman commented Nov 6, 2020 •

edited

Loading