Implement the Cleaner for kotlin Objects

[jhugman]

This PR adds:

• java.lang.ref.Cleaner to perform cleanup of Rust Arcs shortly after kotlin Garbage Collection.
• An Android specific cleaner, with configuration to switch between them.

It leaves intact the AutoCloseable infrastructure previously used, so as to allow developers the choice of

1. Prompt clean up when they are finished with the object
2. An automatic backup, sometime later.

Some further work may be necessary:

1. removing Disposable in favour of java.lang.AutoCloseable.
2. maybe: putting AutoCloseable behind a [Traits=] trait declaration.

For ease of review, and backwards compatibility, I'd suggest keeping these separate from this PR.

[badboy]

I guess there's no guarantee that the GC actually ran at this point, right? Will this lead to annoying intermittents on CI?

[jhugman]

Perhaps. On my slow machine, this test never failed.

The test failure on first push (not this one) suggested that the thread was working rather better on CI than my machine.

The alternative: putting in ~500ms sleep– which I was trying to avoid.

[jmartinesp]

What about manually calling System.gc() before? It doesn't actually guarantee the GC will be done at that point, but it should make it more likely.

EDIT: actually, I just saw this comment, but I don't see why this should be a problem in a test. If you want to have reliable tests for GC, then you should probably force a GC pass.

[bendk]

Looks good to me. The only reason I didn't check approve is I also wonder about the tests failing intermittently.

[bendk]

Nit: I like using Uniffi whenever we need a unique prefix.

[bendk]

I prefer relying on 2 and would love to get rid of the destroy() method. It gets around corner cases and feels more consistent with GC-based language.

How serious is the risk of cleaner thread starvation? I hope it's something most applications can ignore.

I don't think this needs to be a blocker. I'd be happy to merge this one as-is and discuss in a future PR.

[jhugman]

(1) is the escape hatch for when thread starvation happens. As you say, most apps won't have to worry about it.

We should deprecate and then remove the Disposable interface, and the destroy method, though this isn't done here, because we should go through the formal deprecation process.

[jhugman]

Ok, all checks passing.

[jmartinesp]

I tested this on our Android app and the app instantly crashed.

It turns out both java.lang.ref.Cleaner and android.system.SystemCleaner are only available from API 33+, which is only used in around 36% of all available devices. Given some input from my team, I thought this class would be desugared and added to our Kotlin sources from the Java 9+ APIs, but it turns out it's not. However, JNA does have its own Cleaner class (com.sun.jna.internal.Cleaner).

So maybe for the Android version, something like this could be used?

val cleaner = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) {
    SystemCleaner.cleaner()
} else {
    com.sun.jna.internal.Cleaner.getCleaner()
}

The problem with these is, these 2 Cleaner instances come from different classes, so you'd have to either write a wrapper and cast it accordingly or do that in-place for every call to this cleaner. Otherwise, maybe a custom Cleaner implementation based on either java.lang.ref.Cleaner or SystemCleaner could be provided.

[jmartinesp]

In https://github.com/jmartinesp/uniffi-rs/tree/android-cleaner-wrapper I created an example wrapper for Android based on the android_cleaner config value:

• If android_cleaner is disabled, the Java 9+ Cleaner is used (the tests pass, so this should work as expected).
• If android_cleaner is enabled, the AndroidCleaner interface is used instead:
  • On API 33+: AndroidSystemCleaner is implemented, which uses the same Java 9+ Cleaner since it's available.
  • On older APIs, AndroidJnaCleaner is implemented instead, which uses the JNA Cleaner implementation.

I checked both Android implementations work using devices with different API versions, so maybe you can use something like this as a base for the implementation.

[jhugman]

In https://github.com/jmartinesp/uniffi-rs/tree/android-cleaner-wrapper I created an example wrapper for Android based

Oh, amazing. Thank you!

I'm pretty busy this week, so probably won't get to this until next week. Do you want to take this over, or can it wait until then?

[jmartinesp]

I'm pretty busy this week, so probably won't get to this until next week. Do you want to take this over, or can it wait until then?

Take it over as in creating a new PR with these changes plus mine? If so, I think the work should probably continue in this PR, maybe cherry picking my AndroidCleaner implementation if it's good enough.

Also, I don't think anyone is in a hurry for this, since anyone that uses UniFFI with Kotlin is already freeing the memory manually, so we can keep doing that until this is ready. That said, it would be super useful to have it 😅 .

[jhugman]

@jmartinesp would you mind having another look at this sometime? I haven't been able to test it on Android, either with or without android_cleaner = true.

[jhugman]

(1) is the escape hatch for when thread starvation happens. As you say, most apps won't have to worry about it.

We should deprecate and then remove the Disposable interface, and the destroy method, though this isn't done here, because we should go through the formal deprecation process.

[jmartinesp]

Sorry, I deleted the comments above because I just realised I was testing an outdated version of the branch 🤦 .

With the latest commits I can confirm it works both on the newest API (API 34, Android 14) and old ones (API 23, Android 7) using their respective versions of the cleaner.

[jmartinesp]

It also works well with the workaround you added with try/catch when java.lang.ref.Cleaner cannot be found and android and android_cleaner are false, it defaults to the JNA Cleaner in older APIs, and it uses the actual java cleaner when it is available.

However, maybe some kind of message should be logged here? I wouldn't have realised what was happening until I took a look at the implementation details.

[jhugman]

However, maybe some kind of message should be logged here?

There doesn't seem to be anything logged by the Kotlin FFI at this point. Where do you want to log to in (a potentially but not definitely non-Android) context? e.g. java.util.logging.Logger, println.

I'm happy to log if this is a solved problem already, but I'd also be happy to not.

Last commit fixes up documentation.

[jmartinesp]

Where do you want to log to in (a potentially but not definitely non-Android) context? e.g. java.util.logging.Logger, println.

Good question. I think Logger won't work with logcat on Android, so maybe println would be better. In any case, if this creates further problems maybe it's better not to do it until you decide on how to print logs from the library separately.

[bendk]

Looks good to me. I'm mostly relying on @jmartinesp's review, but I'm happy to rubber stamp.

[julian-baumann]

I am getting the following error:

Unresolved reference: Cleaner

in this generated code:

// The fallback Jna cleaner, which is available for both Android, and the JVM.
private class UniffiJnaCleaner : UniffiCleaner {
    private val cleaner = com.sun.jna.internal.Cleaner.getCleaner()

    override fun register(value: Any, cleanUpTask: Runnable): UniffiCleaner.Cleanable =
        UniffiJnaCleanable(cleaner.register(value, cleanUpTask))
}

private class UniffiJnaCleanable(
    private val cleanable: com.sun.jna.internal.Cleaner.Cleanable,
) : UniffiCleaner.Cleanable {
    override fun clean() = cleanable.clean()
}

I have set android = true and android_cleaner = true in my uniffi.toml config, but the issue persists. Do I have to add some sort of dependency for com.sun.jna.internal.Cleaner?

[jmartinesp]

JNA's Cleaner was added in v5.12.0 if I'm not mistaken. Could your JNA version be previous to that one?

EDIT: I just realised this project sets a min version for JNA of 5.7.0, I didn't realise that when I was testing this, sorry. We've always used a more recent version in our project.

I guess either the docs must be updated to change the new min version or the implementation must change.

[mhammond]

I previously had my classpath pointing at a JNA which came with Android Studio, which presumably was an older version (the filename had no version - just jna.jar). I then downloaded jna-5.14.0.jar and pointed at that, which got past my compiler errors but now I always get:

java.lang.AssertionError: Num alive is 1002. GC/Cleaner thread has starved
        at Test_coverall.<init>(test_coverall.kts:532)
test uniffi_foreign_language_testcase_test_coverall_kts ... FAILED

I'm not sure if this is just another example of "tests which rely on clocks etc are always going to fail" like a few others we have (I did make one feeble attempt at sleeping longer, but couldn't make it work), or there is something else going on, but now I just tend to run UniFFI with kotlin disabled :(

[mgeisler]

Hi there 👋

I'm looking to use UniFFI for a library which will be used in Android. The paragraph here gave me pause: will an application actually leak memory in an unrecoverable way, or will it "just" hold on to the memory longer than necessary?

I'm asking since the preconditions seem to be that drop is slow or that it takes a long time to cleanup all the objects. That makes me think that the app will be slower than ideal, but that it will still (eventually) free the memory?

Even if the phone runs out of RAM, the OS will eventually clean things up as mentioned further up in the comment.

What am I missing here?

[mhammond]

I'm far far from an expert in Kotlin, but my understanding of the situation is that the memory held by the Rust object will not be free'd until the generated object's generated destroy() method is called. However, the java garbage collector is both non-deterministic and makes no real attempt to keep memory usage low. The "modern" way of cleaning stuff up is apparently via this "cleaner" mechanism, but even this isn't perfect as it runs on its own thread and nothing guarantees that thread doesn't get starved. In that situation you will have what looks very much like a leak even though it's not really a leak - there's nothing stopping these things getting collected, it's just that they have not been.

Firefox for Android doesn't really exercise much of this - the objects we use tend to be long lived and small in number, whereas most data is stored in records which doesn't have this consideration as records don't have an "attached" Rust object.

[mgeisler]

Hi @mhammond, thanks a lot for the quick reply!

I'm far far from an expert in Kotlin,

Me neither, for the record 😅 I learned Java some 20 years ago and I broadly hope Kotlin is a nicer version of that.

In that situation you will have what looks very much like a leak even though it's not really a leak - there's nothing stopping these things getting collected, it's just that they have not been.

That makes sense! Perhaps it's just a small matter of terminology: I'm used to thinking of a leak as "ups, I threw away the pointer!" and as a result, the allocation cannot be freed.

The indeterministic behavior of a garbage collector is something else for me — even if it results in the program using more memory than intended.

I guess a reference cycle in Rust also falls under the same category of leaks now that I think of it.

[mhammond]

That makes sense! Perhaps it's just a small matter of terminology: I'm used to thinking of a leak as "ups, I threw away the pointer!" and as a result, the allocation cannot be freed.

The indeterministic behavior of a garbage collector is something else for me — even if it results in the program using more memory than intended.

Me too! However, our Android engineers are very vocal in treating it far more seriously than that. I can only assume that's because so many bugs in our Android apps are due to our processes being killed due to memory pressure - I guess Android doesn't simply run a GC to try and reclaim memory before unilaterally killing it.

[mgeisler]

I see... I actually work in Android, but in the Rust part, so I don't really know anything about how apps work. But I can see how people will want stern warnings about this when it leads to the app being killed at random times.

Thanks for clearing it up for me!