Remove support for non-threadsafe interfaces. #452

rfk · 2021-05-21T06:11:01Z

Fixes #449. Depends on #451. This will be a draft until we've given the v0.9.0 release a chance to roll out to consumers.

With the v0.9.0 release of UniFFI, non-threadsafe interfaces are explicitly deprecated. This commit removes support for non-threadsafe interfaces entirely, making threadsafety the default and only supported mode. In practice, for any interface declared in the UDL:

Its implementation must be Sync and Send.
Its methods cannot accept a mutable &self parameter.

The [Threadsafe] attribute may still be specified explicitly in the UDL file, but will result in a deprecation warning and is otherwise ignored.

We don't have a good way to write tests for the following, but I've manually verified that:

A non-Send+Sync interface implementation produces a compile-time error.
Use of the [Threadsafe] attribute results in a deprecation warning.

rfk · 2021-05-21T06:12:06Z

fixtures/coverall/tests/bindings/test_coverall.kts

+// to the shared counter object.
+
+ThreadsafeCounter().use { counter ->


I removed some layers of abstraction from this test, and moved it from "examples" to "fixtures". The abstractions seemed less necessary now that it's not trying to test both the threadsafe and non-threadsafe scenarios.

rfk · 2021-05-21T06:13:18Z

This is ready for review, but I don't plan to land it with any urgency. My main motivation here is to simplify work on the Arc-pointers effort, and in particular, to simplify the docs will that will need to be written for that effort. I'm not really interested in trying to explain a different version of the automagic-mutex behaviour in addition to rambling on about Arcs.

rfk · 2021-05-21T06:45:26Z

docs/manual/src/internals/object_references.md

+from foreign languages on any thread, outside of the control of Rust's ownership
+system. UniFFI itself tries to take a hands-off approach as much as possible and
+depends on the Rust compiler itself to uphold safety guarantees, without assuming
+that foreign-language callers will be "well behaved".


@mhammond FYI I've liberally appropriated the docs updates from your #420.

great - so the nits below are probably mine :)

mhammond

Lovely!

mhammond · 2021-05-21T06:33:36Z

docs/manual/src/internals/object_references.md

+Typically this will mean that the Rust implementation of an object uses some of Rust's
+data structures for thread-safe interior mutability, such as a `Mutex` or `RwLock` or
+the types from `std::atomic`. The precise details are completely up to the author
+of the component - as much as possible, UniFFI tries to stay out of your way, simply requiring


mhammond · 2021-05-21T06:39:24Z

docs/manual/src/internals/object_references.md

-  underlying struct to use interior mutability and manage its own locking.
-* The `ArcHandleMap` can only contain structs that are `Sync` and `Send`, ensuring that
-  shared references can safely be accessed from multiple threads.
+  by enforcing that the contained type is `Send+Sync`, and by refusing to hand out any mutable references.


s/enforcing that the contained type is/specifying that the contained type must be/ or something? "enforcing" makes it sound like some kind of check the map itself is doing, but it's the compiler that is actually enforcing.

mhammond · 2021-05-21T06:40:32Z

docs/manual/src/udl/interfaces.md

@@ -40,11 +40,9 @@ interface TodoList {

 By convention, the `constructor()` calls the Rust's `new()` method.

-Conceptually, these `interface` objects are live Rust objects that have a proxy on the foreign language side; calling any methods on them, including a constructor or destructor results in the corresponding methods being call in Rust.
+Conceptually, these `interface` objects are live Rust structs that have a proxy object on the foreign language side; calling any methods on them, including a constructor or destructor results in the corresponding methods being call in Rust.


typo: called

mhammond · 2021-05-21T06:41:46Z

docs/manual/src/udl/interfaces.md

-`uniffi` will generate these proxies of live objects with an interface or protocol.
-
-e.g. in Kotlin.
+UniFFI will generate these proxies of live objects with an interface or protocol to help with testing in the foreign-language code. For example in Kotlin, the `TodoList` would generate:


nit: "of live objects" can probably be dropped from this sentence?

mhammond · 2021-05-21T06:43:15Z

examples/sprites/src/lib.rs

@@ -31,8 +31,8 @@ pub fn translate(p: &Point, v: Vector) -> Point {
 // and which can move about over time.
 #[derive(Debug)]
 pub struct Sprite {
-    // All interfaces must be threadsafe, meaning `Send+Sync`, and our naive
-    // `Point` struct isn't - so we wrap it in a `RwLock` to make it so.
+    // All interfaces must be `Send + Sync`, and our naive `Point` struct


nit: almost everywhere else has no space between Send and Sync. Not sure which one is actually preferred, but we might as well be consistent.

mhammond · 2021-05-21T06:46:41Z

docs/manual/src/internals/object_references.md

+from foreign languages on any thread, outside of the control of Rust's ownership
+system. UniFFI itself tries to take a hands-off approach as much as possible and
+depends on the Rust compiler itself to uphold safety guarantees, without assuming
+that foreign-language callers will be "well behaved".


great - so the nits below are probably mine :)

README.md

Fixes #449. With the v0.9.0 release of UniFFI, non-threadsafe interfaces are explicitly deprecated. This commit removes support for non-threadsafe interfaces entirely, making threadsafety the default and only supported mode. In practice, for any `interface` declared in the UDL: * Its implementation must be `Sync` and `Send`. * Its methods cannot accept a mutable `&self` parameter. The `[Threadsafe]` attribute may still be specified explicitly in the UDL file, but will result in a deprecation warning and is otherwise ignored. We don't have a good way to write tests for the following, but I've manually verified that: * A non-`Send+Sync` interface implementation produces a compile-time error. * Use of the `[Threadsafe]` attribute results in a deprecation warning. Co-authored-by: Neil P. <npars@users.noreply.github.com> Co-authored-by: Mark Hammond <mhammond@skippinet.com.au>

rfk · 2021-05-27T04:33:14Z

Let's push ahead with this; if we feel like we need further releases in the v0.10 series we can manage them on a branch.

rfk commented May 21, 2021

View reviewed changes

rfk force-pushed the i449-interfaces-always-threadsafe branch from 30d0311 to a90232a Compare May 21, 2021 06:43

rfk force-pushed the fix-kt-optional-uint branch from 04ca542 to 926e858 Compare May 21, 2021 06:43

rfk force-pushed the i449-interfaces-always-threadsafe branch from a90232a to 791f89d Compare May 21, 2021 06:44

rfk commented May 21, 2021

View reviewed changes

mhammond approved these changes May 21, 2021

View reviewed changes

rfk force-pushed the i449-interfaces-always-threadsafe branch from 791f89d to 82dbf73 Compare May 21, 2021 07:13

npars reviewed May 21, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

rfk force-pushed the fix-kt-optional-uint branch from 926e858 to e6bc3ef Compare May 24, 2021 10:06

rfk force-pushed the i449-interfaces-always-threadsafe branch 2 times, most recently from 65c6aef to 199b8a9 Compare May 24, 2021 22:29

Base automatically changed from fix-kt-optional-uint to main May 25, 2021 00:44

rfk force-pushed the i449-interfaces-always-threadsafe branch 3 times, most recently from 324494a to 2a958c7 Compare May 25, 2021 04:38

rfk mentioned this pull request May 25, 2021

Allow passing object instances as args, fields and return values. #462

Merged

rfk force-pushed the i449-interfaces-always-threadsafe branch from 2a958c7 to fa7aa1f Compare May 26, 2021 03:10

rfk force-pushed the i449-interfaces-always-threadsafe branch from fa7aa1f to 52ef91a Compare May 26, 2021 04:36

rfk marked this pull request as ready for review May 27, 2021 04:32

rfk merged commit cde2489 into main May 27, 2021

rfk deleted the i449-interfaces-always-threadsafe branch May 27, 2021 04:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove support for non-threadsafe interfaces. #452

Remove support for non-threadsafe interfaces. #452

rfk commented May 21, 2021

rfk May 21, 2021

rfk commented May 21, 2021

rfk May 21, 2021

mhammond May 21, 2021

mhammond left a comment

mhammond May 21, 2021

mhammond May 21, 2021

mhammond May 21, 2021

mhammond May 21, 2021

mhammond May 21, 2021

mhammond May 21, 2021

rfk commented May 27, 2021

		// to the shared counter object.

		ThreadsafeCounter().use { counter ->

Remove support for non-threadsafe interfaces. #452

Remove support for non-threadsafe interfaces. #452

Conversation

rfk commented May 21, 2021

Choose a reason for hiding this comment

rfk commented May 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhammond 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

rfk commented May 27, 2021