Add destructors for objects #196

jhugman · 2020-08-03T16:47:59Z

Fixes #8.

This PR adds deinit and __del__ calls into a reserved object_free method. (aside: this should be one more example for the need for #10.)

For kotlin, the JVM has a long and storied history of destructor not being the right thing.

Our best hope is for java.lang.ref.Cleaner to be implemented in Java 9 and then adopted by Android.

Our second best hope is for explicit destroy method. Provided here is an abstract class which checks if the object has been destroyed before each call.

jhugman

I've left a few questions as a self-review.

jhugman · 2020-08-03T16:50:27Z

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

+inline fun <T : FFIObject, R> T.use(block: (T) -> R) =
+    try {
+        block(this)
+    } finally {
+        try {
+            this.destroy()
+        } catch (e: Throwable) {
+            // swallow
+        }
+    }


We all liked use for testing, so I implemented it here.

jhugman · 2020-08-03T16:52:31Z

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

+        if (!isDestroyed.get()) {
+            block()


We could do better protecting this critical section; not sure it's needed right now.

jhugman · 2020-08-03T16:55:51Z

uniffi/src/interface/mod.rs

+                ffi_func: Default::default(),
+            },
+        }
+    }


🦀 question: Should this be new? By convention, does new have arguments?

I think this should be new, and it's fine for new to have arguments if they're obviously needed for making an instance.

However, I don't think you'll need this at all if you can avoid creating an API-level Method for this, and use only an FFIFunction in the style of the bytebuffer helpers.

I added new.

However, avoiding adding an reserved Method meant that I was passing in the namespace into the Object now.

jhugman · 2020-08-03T16:59:50Z

uniffi/src/interface/mod.rs

@@ -166,7 +166,7 @@ impl<'ci> ComponentInterface {

    pub fn ffi_string_free(&self) -> FFIFunction {
        FFIFunction {
-            name: format!("{}_string_free", self.namespace()),
+            name: format!("_{}_string_free", self.namespace()),


Giving these prefixes makes it harder to use uniffi to write collisions with other uniffi generated code.

I'm told that names with a leading underscore are reserved for use by the compiler in C code. I don't have any particular case in mind where this would cause problems for us, but one alternative might be to put the leading underscore after the namespace, like {}__bytebuffer_free.

The extra character needs to be before the namespace, otherwise collisions are possible.

Assuming adding a namespace-trailing underscore, this IDL has two collisions.

namespace { void _string_free() void Sprite__object_free() } class Sprite { }

I'll change this to something more verbose/less exotic.

uniffi/src/interface/mod.rs

jhugman · 2020-08-03T17:31:34Z

uniffi/src/bindings/kotlin/templates/ObjectTemplate.kt

-    private var handle: AtomicLong = AtomicLong(handle)
-    {%- for cons in obj.constructors() %}
+class {{ obj.name()|class_name_kt }}(
+    internal val handle: Long


handle is never going to change, so removed AtomicLong.

This makes sense to me, but I do wonder why we tend to use AtomicLong for handles in Application Services 🤔

Now changed back to AtomicLong, so we can zero out the handle when we're destroying it.

jhugman · 2020-08-03T20:08:43Z

uniffi/src/templates/ObjectTemplate.rs

+    {% let ffi_free = obj.reserved_object_free().ffi_func() -%}
+    #[no_mangle]
+    pub extern "C" fn {{ ffi_free.name() }}(
+        {%- call rs::arg_list_rs_decl(ffi_free.arguments()) %}) {
+        log::debug!("{{ ffi_free.name() }}");
+        let _retval = {{ handle_map }}.delete_u64(handle);
+    }
+


This should be replaced with

define_handle_map_deleter!({{ handle_map }}, {{ ffi_free.name() }})

once #31 lands and I can cargo cult the how that works better.

rfk

Thanks @jhugman! This looks pretty solid on a first pass, I left a couple of initial comments and will come back to this in more detail later.

rfk · 2020-08-03T22:17:19Z

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

+// This would be a good candidate for isolating in its own ffi-support lib.
+
+abstract class FFIObject {
+    internal val isDestroyed = AtomicBoolean(false)


Something we've done in the hand-written bindings, was use this.handle == 0 to indicate that the object is dead. Would it make sense to move handle up to a property of this parent class in order to do something like that?

Rewritten in terms of AtomicLong.

rfk · 2020-08-03T22:20:51Z

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

+        isDestroyed.set(true)
+    }
+
+    internal inline fun <R> safelyCall(block: () -> R) =


Naming nit: things with "safe" in their name are a personal bugbear of mine because it's often not obvious what "safe" means and they regularly turn out to be less safe than advertised. It's probably not a big deal for an internal helper function like this, but still, is there a more descriptive name?

One possible suggestion: if we moved handle up to be a property of this parent class, you could make this something like callWithHandle. Not sure if that would make the call-side code in each method declaration messier though.

rfk · 2020-08-03T22:22:59Z

uniffi/src/bindings/kotlin/templates/ObjectTemplate.kt

-    // XXX TODO: destructors or equivalent.
+    override fun destroy() {
+        super.destroy()
+        {% call kt::to_rs_call_with_prefix("this.handle", obj.reserved_object_free()) %}


Are we are deliberately calling super.destroy() first so that we "poison" the Kotlin side of the object, as a guard against potential concurrency issues. I expect that the underlying handlemap will prevent concurrency issues in practice, but still, if the ordering here is important it may be worth a comment to that effect.

rfk · 2020-08-03T22:26:28Z

uniffi/src/bindings/python/templates/ObjectTemplate.py

@@ -6,7 +6,8 @@ def __init__(self, {% call py::arg_list_decl(cons.arguments()) -%}):
        self._handle = {% call py::to_rs_call(cons) %}
    {%- endfor %}

-    # XXX TODO: destructors or equivalent.
+    def __del__(self):


Should this similarly poison the object on the python side, like we do in Kotlin? IIRC it is possible that someone might subclass one of these and extend __del__ in a way that resurrects the the python-side object after the rust-side destructor has been called. (They really shouldn't do that, but it's possible).

Ug. I think I'm going to file a python-only issue to investigate stopping resurrecting python side objects.

rfk · 2020-08-03T22:31:23Z

uniffi/src/interface/mod.rs

@@ -166,7 +166,7 @@ impl<'ci> ComponentInterface {

    pub fn ffi_string_free(&self) -> FFIFunction {
        FFIFunction {
-            name: format!("{}_string_free", self.namespace()),
+            name: format!("_{}_string_free", self.namespace()),


I'm told that names with a leading underscore are reserved for use by the compiler in C code. I don't have any particular case in mind where this would cause problems for us, but one alternative might be to put the leading underscore after the namespace, like {}__bytebuffer_free.

rfk · 2020-08-03T22:36:36Z

uniffi/src/interface/mod.rs

@@ -533,9 +534,24 @@ pub struct Object {
    name: String,
    constructors: Vec<Constructor>,
    methods: Vec<Method>,
+    object_free: Method,


Does this need to be an API-level Method? Naively, it feels like it would suffice to generate just the FFIFunction and rely on the automatically-generated foreign-language code for the API-level definition.

In practice that would look very similar to how bytebuffer_free and friends work on the ComponentInterface:

The Object::reserved_object_free method creates a new FFIFunction every time it's called.

The Object::iter_ffi_function_definitions method calls Object::reserved_object_free and appends the result to the vec of FFIFunction definitions.

For foreign-language templates directly emit an FFI-level call to the function returned by Object::reserved_object_free.

I think that would be all you need, and would simplify some of the changes here.

I've moved this as directed here; it did mean touching the foreign-language macro templates for all functions and methods, and adding a method_arguments() method to FFIFunction which elides the first argument, and adds a namespace property for Object.

Is it now safe to remove this object_free property on the struct?

rfk · 2020-08-03T22:39:17Z

uniffi/src/interface/mod.rs

+                ffi_func: Default::default(),
+            },
+        }
+    }


I think this should be new, and it's fine for new to have arguments if they're obviously needed for making an instance.

However, I don't think you'll need this at all if you can avoid creating an API-level Method for this, and use only an FFIFunction in the style of the bytebuffer helpers.

tarikeshaq

This mostly looks good to me 😄 dropped a couple of comments I had in mind

tarikeshaq · 2020-08-04T16:15:08Z

uniffi/src/bindings/kotlin/templates/ObjectTemplate.kt

-    private var handle: AtomicLong = AtomicLong(handle)
-    {%- for cons in obj.constructors() %}
+class {{ obj.name()|class_name_kt }}(
+    internal val handle: Long


This makes sense to me, but I do wonder why we tend to use AtomicLong for handles in Application Services 🤔

tarikeshaq · 2020-08-04T16:36:15Z

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

+    internal val isDestroyed = AtomicBoolean(false)
+
+    open fun destroy() {
+        isDestroyed.set(true)


I'm wondering if we should throw an error here if the object has already been destroyed. Can't think of a concrete reason why, but one thought I'm having is if someone calls the destroy multiple times, I'm guessing we would give them an error, but is it better to error here? And what kind of error would they get if the call goes through the FFI?

I guess we could do what the other bindings do, would Swift/Python complain if you call the destructor twice? Or would they just pass it into the FFI regardless?

As @rfk says, the ConcurrentHandleMap should handle that for us.

I'm torn here: should we be adding isDestroyed so we can encourage/enable defensive programming? Or should we make it idempotent? Or should we just go with what we've got, and review ergonomics once we've started using it?

I like how you've done it in the PR, making destroy idempotent, and throwing if you try to call any methods after destroying the class. It feels like that'll be easier for a-c or whoever to wire it up to Cleaner if they want, without having to do additional state tracking.

tarikeshaq · 2020-08-04T16:47:20Z

examples/sprites/tests/bindings/test_sprites.kts

@@ -11,3 +11,11 @@ assert( s.getPosition() == Point(1.0, 2.0) )

 s.moveBy(Vector(-4.0, 2.0))
 assert( s.getPosition() == Point(-3.0, 4.0) )
+
+s.destroy()


I'm wondering where we'd put documentation for this, so consumers know they need to call the destroy function to do the deallocation. Thoughts?

Oh that's a really interesting question.

For now, I've documented in the generated section, however, we do need a better solution for this.

tarikeshaq · 2020-08-04T16:52:15Z

uniffi/src/templates/ObjectTemplate.rs

@@ -6,41 +6,47 @@
 // If the caller's implementation of the struct does not match with the methods or types specified
 // in the IDL, then the rust compiler will complain with a (hopefully at least somewhat helpful!)
 // error message when processing this generated code.
-
+{% let handle_map = format!("UNIFFI_HANDLE_MAP_{}", obj.name().to_uppercase()) %}


linabutler · 2020-08-04T22:35:23Z

uniffi/src/interface/mod.rs

+    pub fn method_arguments(&self) -> Vec<&Argument> {
+        let args = self.arguments.get(1..self.arguments.len());
+        match args {
+            None => Vec::new(),
+            Some(args) => args.iter().collect(),
+        }
+    }


Suggested change

pub fn method_arguments(&self) -> Vec<&Argument> {

let args = self.arguments.get(1..self.arguments.len());

match args {

None => Vec::new(),

Some(args) => args.iter().collect(),

}

}

pub fn method_arguments(&self) -> &[Argument] {

self.arguments.get(1..self.arguments.len()).unwrap_or_default()

}

Super small nit, but maybe we can avoid the copy and match here?

@linacambridge Oh that's soo much nicer! Thank you!

linabutler · 2020-08-04T22:40:43Z

uniffi/src/templates/ObjectTemplate.rs

+    pub extern "C" fn {{ ffi_free.name() }}(
+        {%- call rs::arg_list_rs_decl(ffi_free.arguments()) %}) {
+        log::debug!("{{ ffi_free.name() }}");
+        let _retval = {{ handle_map }}.delete_u64(handle);


Nit: let _ = ... or drop(...)?

linabutler · 2020-08-04T22:58:55Z

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

+    internal val isDestroyed = AtomicBoolean(false)
+
+    open fun destroy() {
+        isDestroyed.set(true)


I like how you've done it in the PR, making destroy idempotent, and throwing if you try to call any methods after destroying the class. It feels like that'll be easier for a-c or whoever to wire it up to Cleaner if they want, without having to do additional state tracking.

linabutler

Cool!

rfk

This LGTM, thanks for digging in to the FFIFunc/Method changes. I think they will mesh nicely with the more deliberate separation between API-level and FFI-level interfaces that I've been working on over in #192

rfk · 2020-08-05T03:20:24Z

uniffi/src/interface/mod.rs

@@ -548,6 +572,20 @@ impl Object {
        self.methods.iter().collect()
    }

+    pub fn ffi_object_free(&self) -> FFIFunction {
+        FFIFunction {
+            name: format!("ffi_{}_{}_object_free", self.namespace, self.name),


Ah yes, I see the trick here with needing more of the global context for the namespace. This seems OK I think, thanks for digging in.

rfk · 2020-08-05T03:24:46Z

uniffi/src/interface/mod.rs

@@ -533,9 +534,24 @@ pub struct Object {
    name: String,
    constructors: Vec<Constructor>,
    methods: Vec<Method>,
+    object_free: Method,


Is it now safe to remove this object_free property on the struct?

jhugman · 2020-08-05T15:15:56Z

#203 and #31 takes the templates in a different direction. :(

jhugman changed the base branch from master to main August 3, 2020 16:48

jhugman commented Aug 3, 2020

View reviewed changes

jhugman requested review from tarikeshaq and rfk August 3, 2020 20:05

jhugman commented Aug 3, 2020

View reviewed changes

rfk reviewed Aug 3, 2020

View reviewed changes

tarikeshaq reviewed Aug 4, 2020

View reviewed changes

jhugman mentioned this pull request Aug 4, 2020

Investigate preventing resurrection of python objects #201

Open

linabutler reviewed Aug 4, 2020

View reviewed changes

linabutler approved these changes Aug 4, 2020

View reviewed changes

rfk approved these changes Aug 5, 2020

View reviewed changes

Add destructors for objects

96c588c

Fixup to work with new errors in PR mozilla#31

e90e748

jhugman force-pushed the jhugman/8-add-destructors branch from 8fbd373 to e90e748 Compare August 5, 2020 16:36

jhugman merged commit f727a57 into mozilla:main Aug 5, 2020

jhugman deleted the jhugman/8-add-destructors branch August 5, 2020 16:40

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

Investigate preventing resurrection of python objects mozilla/application-services#4255

Closed

Add destructors for objects #196

Add destructors for objects #196

Conversation

jhugman commented Aug 3, 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

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

rfk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhugman Aug 4, 2020 • edited Loading

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 Aug 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarikeshaq 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

jhugman Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linabutler left a 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

jhugman commented Aug 5, 2020

jhugman Aug 4, 2020 •

edited

Loading

jhugman Aug 4, 2020 •

edited

Loading

jhugman Aug 5, 2020 •

edited

Loading