ADR-0008: Freeing objects and resources

[bendk]

We've been discussing this one quite a bit lately and it seems like a great time to make a decision. Here's the start of an ADR which lays out all the possibilities I've heard so far, I'd love to hear what others think. I'm including everyone that I can remember having a discussion about this, but all voices are welcome.

[bendk]

Sammy suggested this one to me recently and I have to admit that I'm biased towards it. Please point out anything I'm missing because of that bias, but I like it because a) it's the least opinionated and b) AFAICT, it's the only solution that also works for callback interfaces.

[bendk]

IMO at least, but this isn't a hard requirement. We could chose one option for objects and a different option for callback interfaces (although trait interfaces could get tricky).

[jplatte]

I'm actually against this, I think in contexts where RAII is available, we should use it. But maybe I'm misunderstanding something as I wasn't even aware that freeing of callback interface objects was ever a problem, and this ADR doesn't really provide any details as to why it's a problem (I would have thought dropping the Box<dyn Trait> would do the right thing,though now I'm wondering how it's even possible that Rust gets unique ownership over such an object, I guess it actually doesn't and the Box points to a GC-managed object?).

[bendk]

Exactly. A callback interface can hold a resource. With the current code, when you drop the Rust object then we will drop the GC-reference, but that doesn't mean the resource will be released. Best-case scenario it will be released at some time in the future. Worst-case scenario it won't be cleaned up at all, because the Kotlin user expected an explicit close() method to be called. In practice, is the library would probably call that explicit close() method before dropping the Box, which is why I'm thinking maybe we should just make that the norm.

That said, @arg0d points out that you could connect dropping the Box to a close() method. I'm going to update the ADR with that option.

[jplatte]

Why are memory allocations not mentioned? Seems like the most frequent / obvious kind of resource basically every UniFFI API handles.

[bendk]

I always think the same thing, but most Java/Kotlin/Python programmers make a distinction between memory and other types of resources. Memory is abundant enough for the GC to clean up, other resources are scarce and should be cleaned up immediately.

I don't think there's a hard line here though, if you allocate a gig of memory then that should probably be considered a "scarce resource" and if we picked option 1, the close() method would free that memory.

[arg0d]

Java/Kotlin/Python programmers make a distinction between memory and other types of resources

this sentence clears up ideas in this ADR, you should include it in the context.

[linabutler]

I also had the same question at first, and wanted to share how I thought through it, in case it's helpful for context or other folks!

IIUC, from the perspective of the foreign code, a UniFFI object is "a pointer to some memory that exists outside of the GC heap". Because it's outside the GC heap, it can't be GCed automatically.

That's why these objects need some kind of destructor, either:

1. An explicit method, like close() or destroy(), or...
2. A finalizer that's automatically called by the foreign language's runtime. In the JVM, that's java.lang.Object.finalize() or Cleaner; in Python, that's __del__; in Go, that's runtime.SetFinalizer, and so on.

On the Rust side, the object might have a handle to a "resource"—like a database connection, a file handle, etc.—or it might not, but that's totally opaque to the foreign code.

In that sense, I don't think UniFFI's AutoCloseable machinery today is concerned about "resources" specifically—it's a destructor that tells Rust to drop the object, and free up the memory it uses. Whether or not dropping the object also happens to release "resources" on the Rust side—for example, a rusqlite connection implementing Drop to automatically close the connection—is an implementation detail from Kotlin's perspective.

[jplatte]

Don't all objects need to be deallocated to avoid leaking memory?

[jplatte]

Idk what "see #1394" is supposed to do here, it has lots of comments and I didn't find any obvious "using Gc would be wrong" statements there so far, just lots of discussions about GC as the only way of cleaning up being insufficient in some cases.

[bendk]

I linked #1394 for the opposite reason: to show that sometimes we should rely on the GC. The sentence is confusing because there's a weird double-negative in there though. I'll try to clean that up.

[jplatte]

I'd say it also has the downside of adding more complexity. The other two options don't really introduce any new concepts in the public API from my POV, this one does.

[jplatte]

I'm actually against this, I think in contexts where RAII is available, we should use it. But maybe I'm misunderstanding something as I wasn't even aware that freeing of callback interface objects was ever a problem, and this ADR doesn't really provide any details as to why it's a problem (I would have thought dropping the Box<dyn Trait> would do the right thing,though now I'm wondering how it's even possible that Rust gets unique ownership over such an object, I guess it actually doesn't and the Box points to a GC-managed object?).

[arg0d]

Foreign object holding a resource would be a callback implementation on foreign language side, right? Any resources being held by the callback are user defined, right? So it should be possible to generate a "close" method for the callback that is called when Rust invokes callback with 0 to remove the callback from handle map. User could then implement the appropriate logic to cleanup any resources being held by the callback. Closing the callback seems to be already implemented, only close method is lacking in the interface to allow foreign bindings to close further resources.

[bendk]

Good point, I'm going to update the docs with this option.

[mhammond]

Foreign object holding a resource would be a callback implementation on foreign language side, right?

I think we want to generalize callbacks into more generic traits though - eg, wrap them in an Arc<> such that Rust code can be blissfully unaware whether a trait implementation is foreign or rust implemented.

[mhammond]

Thanks heaps for driving this forward Ben - it's a discussion we keep having that never seems to make progress!

Initially I'm just putting down some thoughts on the problem statement, as I believe tightening this up might help the rest.

#1394 says we could add "finalizer support to make a 'best effort' attempt at freeing what it can while leaving the auto-closeable pattern in place" - I'm not aware of us exploring that, nor how this ADR might change if that turned out to be successful.

This section starts off talking about all bindings in general, but then fairly quickly pivots into just Kotlin. While Kotlin is important, I think it's still worth a bit more of an explanation about how the other "important" bindings work. For example, it seems that Python also shares the same general problem - a context-manager is available, but this doesn't really work for long lived references. #8 implies Swift does not have this problem. C# and Go seem worth mentioning too as I suspect they might end up being very popular UniFFI bindings.

Upon re-reading this ADR, I think your "Option 1" can be restated as "Do nothing. It is up to the designer of the API exposed via UniFFI to take care of this. An option might be to add explicit "close()" methods, but that's all on you, we don't care". If that's accurate, maybe update the wording to be more explicit/brutal? We've also been talking about "transitive requirements (eg, enum holding a dictionary which holds a resource - there's nothing generic you can do on the enum to clean this tree up) - if the intention is also to offer no help there and leave that to the API designer it might be worth calling that out.

I also don't really understand the callback example - or more specifically, how the callback example is any different from the general problem. If you consider a pure-python program which takes some arbitrary object, there's also no generic way for the holder of that object to release the resources held by that arbitrary object - but that's not generally considered a problem - the solutions are either (a) know the type, so, eg, if it's a file you know you can .close() it, or (b) don't do that. IOW, the responsibility for this on the designer of the API so that either (a) the API has an explicit close-type method, or (b) don't require the Rust code to keep a long-lived reference to an object holding resources you care about?

tl;dr - I think I'm also in favour of Option 1 if I understand it correctly - and if I do, I'd suggest:

• Making that option even more explicit and "brutal" - ie, explicitly call out "this is your problem, UniFFI doesn't help"
• Explicitly call out the transitive case (ie, if your enums hold objects which manage resources, you are probably going to have a bad time, but UniFFI isn't going to try and stop you)
• Either remove most of the callback language (ie, replace it with something like "this is just a slightly different view of the exact same problem") or clarify how callbacks differ.
• Make the problem statement slightly less Kotlin specific, or explain how, eg, Python is different.

And probably most importantly, work out which Kotlin people to put this in-front of. Most UniFFI core contributors know enough Kotlin to be dangerous, but probably not quite enough to know how viable this will be in practice.

[bendk]

Thanks, I think these are great suggestions. I "brutalized" option 1 a bit, I could have gone farther, but I think it makes the point pretty well now. I added some text about the transitive case and also other languages. I agree it shouldn't be Kotlin specific.

I moved callbacks to their own section and tried to elaborate a bit more. On the one hand, I think that it's all part of the same problem, but I also think they're important because they provide extra complications for any automatically generated solution.

Next step is to find some more Kotlin people...

[jplatte]

I didn't know anything about this API, nor much about "oldschool" finalizers, but https://stackoverflow.com/questions/52879761/should-java-9-cleaner-be-preferred-to-finalization points towards Cleaner really being the best choice here.

[jhugman]

Objects can hold resources that should be cleaned up promptly like database connections, file handles, network sockets, large memory allocations, etc.
Many languages, like Kotlin, Java and Python make a distinction between these types of resources and GC heap allocations that is reflected in the API.
Programmers typically expose a close method and languages often offer constructs to assist in using these like context managers on Python or the use extension function on Kotlin.
However, on Rust and other RAII-based languages, these resources are typically released in the destructor rather than with a dedicated API.

I'm not sure that this is the problem to which the solution proposed is solving.

The problem we have been discussing is this:

• most language expose some kind of finalizer where we can inform Rust to free up memory when the language no longer needs it:
  • In Swift we implement deinit
  • In Python we implement __del__
  • In Ruby we define with ObjectSpace.define_finalizer

In Kotlin, we might implement java.lang.Object.finalize (spec). However:

• it has been deprecated since JDK 9,
• anecdotally, it has been observed as the source of crashes on Android.
• before it was deprecated, it was the subject of heavy warnings against using it– so is a source of JVM diversity and little usage.

Indeed, the Android documentation on java.lang.Object#finalize is very instructive here, and worth quoting extensively:

Note that objects that override finalize are significantly more expensive than objects that don't. Finalizers may be run a long time after the object is no longer reachable, depending on memory pressure, so it's a bad idea to rely on them for cleanup. Note also that finalizers are run on a single VM-wide finalizer thread, so doing blocking work in a finalizer is a bad idea. A finalizer is usually only necessary for a class that has a native peer and needs to call a native method to destroy that peer. Even then, it's better to provide an explicit close method (and implement Closeable), and insist that callers manually dispose of instances. This works well for something like files, but less well for something like a BigInteger where typical calling code would have to deal with lots of temporaries. Unfortunately, code that creates lots of temporaries is the worst kind of code from the point of view of the single finalizer thread.

If you must use finalizers, consider at least providing your own ReferenceQueue and having your own thread process that queue.

For our case, the mitigations from (in chronological order) Josh Bloch's Effective Java, the JDK deprecation of finalize and the Android doc all point in the same direction:

• implement Closeable and AutoCloseable, and insist our users manually dispose of their instances.
• implement a Cleaner, or something else that polls a ReferenceQueue.
• don't rely on either happening as soon as the object drops out of scope.

So far, we have implemented AutoCloseable and Closeable; and this is the source of impedance mismatch and confusion.

1. As @linabutler alludes, JVM heap memory isn't something that Java/Kotlin programmers need to worry about, so most things that needs a finalizer are open and closeable, e.g. sockets, files, databases. However non-heap memory doesn't fall cleanly into either of these categories: it does need cleanup, but isn't openable and closeable. It feels strange to close an only-in-memory object, but that's currently what we advise to avoid memory leaks.

2. To the untrained eye, AutoCloseable seems to do something useful, however it is really only useful in a specific use case: namely interacting with the so-called try-with-resources.

Kotlin doesn't have a try-with-resources, but does implement use which does the same job. The implementation of use is worth looking at, if only to see how little the tooling around AutoCloseable and Closeable gives use.

3. Part of uniffi's value proposition is that we try and expose Rust crates to foreign languages in such a way as using them is indistinguishable from using a foreign language equivalent. We should look at the existence of Closeable and forcing users to explicitly call close on interface structs is a leak in the abstraction we should seek to close.

---

TL;DR:

1. We should move away from using Closeable and AutoCloseable, in favour of a cleaner based approach, to bring up to parity with the other languages we support.
2. We should not replicated close methods for other languages. If __del__ is deprecated in Python we should investigate alternatives (e.g. ContextManager).

WDYT?

[mhammond]

Thanks for the detailed reply James! A couple of questions:

Re (1), I always thought that Closeable and AutoCloseable were not mutually exclusive - so if a Kotlin object holds a database connection, AutoCloseable might be a good way to ensure the database connection is closed even while the object remains reachable. So while I understand the motivation of moving from Closeable to the cleaner, I don't see why it follows that we should also move away from AutoCloseable?

Re (2) I don't quite understand what you are saying there. Eg, re Python, are you proposing something should change re __del__, or just speculating what might happen if __del__ was deprecated? But even if the latter, the reference to a ContextManager confuses me a little - IIUC, a ContextManager is analogous with AutoClosable in that it's a way to force reclamation of resources even while the object remains reachable. IIUC, __del__ being deprecated could not naively be replaced with a context manager in the same way that Kotlin can't rely exclusively on AutoCloseable because, as you mention, it requires the consumer to use a very specific pattern for it to be effective.

Indeed, combining these points, my take away would be:

1. Python already has __del__, Swift already has deinit, the sanest thing Kotlin could do for parity there is the cleaner.
2. For more fine-grained control (ie, so there's a sane path to reclaiming resources even while objects remain reachable) our objects should also implement a ContextManager for Python, AutoCloseable for Kotin, (and ??? for Swift?*)
3. Given Rust uses Arc<>, dropping the Rust reference might not have the desired effect in practice, so we'd encourage UniFFI consumers to implement resource reclamation as part of their API. But (1) and (2) above means it's generally not necessary for every interface exposed via UniFFI to have explicit user-defined "close" methods, just ones with "expensive" resources associated with them.

(*I'm even less familiar with Swift than I am Kotlin, but IIUC swift objects use reference counting, so I can't see why the same arguments don't apply there - I've just no idea if there's a protocol or some other technique that might be suitable)

[bendk]

@mhammond I agree with basically everything you said, except for one part:

For more fine-grained control (ie, so there's a sane path to reclaiming resources even while objects remain reachable) our objects should also implement a ContextManager for Python, AutoCloseable for Kotin, (and ??? for Swift?*)

Given Rust uses Arc<>, dropping the Rust reference might not have the desired effect in practice, so we'd encourage UniFFI consumers to implement resource reclamation as part of their API.

I think there's a tension between these 2 statements. If we're suggesting to the user to define a close()-style method in some cases, then how can we implement Autocloseable for all objects? I think the only way it works is our current system, where we assume that dropping the arc does release the resource. That's likely true for Objects, but once you start mixing in callback interfaces / trait interfaces, I don't think it's true anymore.

I think we should start by simply not trying to implement AutoClosable, ContextManager, etc.

Later on, we could let users opt-in to this with something similar to how we all users to implement standard Rust traits. If an interface defines close and they flag it as being "Closeable", then we implement AutoClosable, ContextManager, etc.

[mhammond]

I think there's a tension between these 2 statements. If we're suggesting to the user to define a close()-style method in some cases, then how can we implement Autocloseable for all objects?

I guess I was suggesting that we can document something simple like: A reference to the corresponding Rust object will be dropped when the object is garbage collected. You can force the reference to be dropped earlier by using the object in a try/with / ContextManager. Note however that dropping the Rust reference doesn't guarantee anything about the Rust object behind the reference, so you are encouraged to add close capabilities to your API".

Later on, we could let users opt-in to this with something similar to how we all users to implement standard Rust traits.

Are they mutually exclusive? We are always going to be dealing with an Arc<>, so I guess I can't really see why a ContextManager can't both call some magic method and drop the reference?

OTOH, I guess an alternative is to stop supporting the ability for the Rust object to be "disconnected" from the foreign object entirely. According to the template code, after Autocloseable::close() is called consumers should expect to see IllegalStateException interacting with the object. Maybe the better thing to do is never actually disconnect and instead let that kind of exception bubble up from the "still connected but now logically closed" Rust object?

(Apologies if that was the intent of James's proposal which I missed!)

[jhugman]

Question (1)

AutoCloseable might be a good way to ensure the database connection is closed even while the object remains reachable

For clarity: we don't need to implement AutoCloseable to do this. AutoCloseable is only to integrate with try-with-resources, which is only good for interacting with in a one-shot-in-the-same-block manner. As far as I can tell, we don't use try-with-resources (a Java only language feature) anywhere in Mozilla.

Similarly, the only thing Closeable does is let us interact with the use function, which is pretty easy to re-implement.

@bendk is correct, both interfaces are almost identical: they each ask you to implement a single close method, which let you interact with a limited set of libraries which don't suit our use case.

Question (2)

I must admit, my python is not what it should be. __del__ looks exactly what we need at the moment. My cursory read of ContextManagers would be that it was slightly more capable as try-with-resources: it offers enter-block and exit-block semantics, where we actually need a longer living lifecycle and orthogonal to blocks.

If __del__ did become deprecated (in favor of something else), I'd guess we'd use the something else, rather than ContextManager.

My point however still stands: we should bring Kotlin support to the same level we have for Python, Swift and Ruby— implicit memory handling is done implicitly, and any use-case-specific resource closing (where necessary/desired) is done explicitly in userland.

Maybe the better thing to do is never actually disconnect and instead let that kind of exception bubble up from the "still connected but now logically closed" Rust object?

This is the point @linabutler so eloquently made this week– by defauit we should avoid the state where we have orphaned/dangling Foreign Language objects that are still reachable, but effectively dead because the backing Rust object has been invalidated by an explict call from the foreign side; this is facility that the users can build for themselves, but we shouldn't enable it by default.

[bendk]

Thanks for all the help on this one everyone. After discussing this more @jhugman and @mhammond, I just pushed an updated ADR (most of it written by @mhammond). The updated ADR:

• Tries to make a clean distinction between the two things we're talking about. "References that need to be released" vs "Resources that need to be closed" was the terminology I went with.
• Discusses container objects as well
• Discusses "foreign sugar", like context managers and Closable / use() a bit, but doesn't try to make decisions on them. Treating that at out-of-scope for the ADR makes a lot of sense to me.
• Focuses more on the general case and less on Kotlin, although there is a specific section on what this means for Kotlin.

[jplatte]

Very nice rewrite of the ADR text!

[mhammond]

Thanks heaps Ben! I have entirely nits (and a type CI job is an interesting idea :)

I guess we also need to consider making a decision here. I believe you and I are clearly on the "option 1" fence, and I doubt that will be controversial, so I guess we should actually fill that part of it in, then get formal approval from a few people. "Deciders" should probably just be something like "Uniffi developers, as approved in PR-XXXX" or similar? Either way, thanks for staying with this!

[bendk]

This is looking great to me. I made the edits based on @mhammond's suggestions, updated the doc to say we decided on option [1], updated the status/deciders lines, etc.

I added those because it feels like we're close to a decision to me, but if anyone has concerns and wants to explore the other options, please add a comment. There's definitely still time to discuss more.

[jhugman]

A few comments on the form and problem statement of the ADR, but nothing to stand in the way of the action proposed.

[jhugman]

Nit: Burying the lede. Considering we don't generate a close method for any other language except Kotlin, and every other language has destructor/finalizer arrangements that we already use, it is surprising that Kotlin is only spoken about in the last 5 lines of the ADR.

[bendk]

I think there's a tension her.e. As a practical matter, this ADR is essentially just suggestion changes to the Kotlin code. As a theoretical matter, I think it's important to discuss the topic in general and separate it from all the details about finalizers, cleaners, etc.

I tried to strike a balance by mentioning this at the top of the doc, do you think that addresses the concern?

[bendk]

I don't think there's been any concerns with the proposal, just suggestions about the ADR wording. If anyone wants to argue for one of the other options, please speak up.

[bendk]

I just realized something about the current Kotlin system: Since user code can cause the underlying handle to be destroyed, it's risky to ever pass it to Rust. Most of the ObjectRuntime.kt file is dedicated to avoiding this for method receivers, but doesn't the same issue apply to function/method arguments? IOW, if I pass an object to a function, what's to stop another thread from calling destroy() while we're in the middle of lowering everything?

[tarikeshaq]

oops not sure why GitHub removed review from @arg0d, wanted remove the request for myself only 😄

[skhamis]

Super late to this party, but option 1 was something I supported and so LGTM!

[linabutler]

Thanks so much, @bendk!

[linabutler]

This is a great summary of the problem! I think we also got confused by the conflation when discussing the options, and this clarifies it really well!

[linabutler]

I really like your separation of references and resources, and having UniFFI release references makes a lot of sense to me! But I'm not sure I follow the conclusion that UniFFI needs to concern itself with resources, and was wondering if we could talk more about that.

If a UniFFI consumer wants to define an interface type T that holds a resource—like a file or a database connection—they could manually define a close method, and have the Rust implementation of T hold a Mutex<Option<Resource>> that the implementation of T::close() method would take() to close the resource early. They could also impl Drop for T, which would take care of closing the resource once the last Arc<T> reference was released.

It's definitely true that, without an opt-in sugar, the UniFFI-generated Kotlin binding for T couldn't implement Closeable or AutoCloseable—T would just have a plain, vanilla close() method. (And, similarly, the Python binding would have a regular close method, and wouldn't be a context manager). But I wonder if that's okay?

I'm also wondering what this opt-in sugar would look like for languages like Swift (and maybe Ruby?) that have different idioms for resources. For example, Swift's deinit is the closest, but ARC doesn't guarantee precise lifetimes, and Swift seems to prefer the "with-prefixed function that takes a closure" idiom for resources (withLock, withExtendedLifetime, withUnsafe{Mutable}{Pointer, Bytes}).

Sorry that was so wordy! I guess I'm wondering—would the opt-in sugar for resources offer enough benefits as a UniFFI feature, than having consumers handle resource management themselves?

[bendk]

Great question. Right now, I fall on the side of probably implementing Closeable and/or AutoClosable on Kotlin, maybe implementing context managers on Python, and that's it. I'm not sure what others feel like or even if that's where I'll end up once we actually test it out.

I think the line that says "The exact mechanics for this are beyond the scope..." was supposed to communicate that we don't really have a final decision here, however the other line does seem to imply that we want to do something. I softened that a bit to just say, we'll investigate it and decide later.

[bendk]

I think one of the last issues is to update this statement so that it's clear that UniFFI would handle this behind-the-scenes and neither the library author or consumer would need to write any code to make that happen. Does this new wording make that clear?

[bendk]

Looks like we're all agreed on the approach. Is there anything outstanding to discuss? If not, I'll aim to merge this tomorrow.

[jplatte]

Not something that should have stopped merging the ADR, but your concern about the Kotlin implementation seems like something that needs more investigation, in case you had forgotten about it again because nobody reacted.

[bendk]

Not something that should have stopped merging the ADR, but your concern about the Kotlin implementation seems like something that needs more investigation, in case you had forgotten about it again because nobody reacted.

I think that gets fixed once freeing the reference is tied to PhantomQueue, Cleaner, or some similar method. Those won't free the reference while it's still reachable, which I think fixes the issue.