Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[Java.Interop] Add JavaPeerableRegistrationScope
Fixes: #4 Context: #426 Context: a666a6f Alternate names? * JavaScope * JavaPeerableCleanupPool * JavaPeerCleanup * JavaReferenceCleanup * JniPeerRegistrationScope Issue #426 is an idea to implement a *non*-GC-Bridged `JniRuntime.JniValueManager` type, primarily for use with .NET Core. This was begun in a666a6f. What's missing is the answer to a question: what to do about `JniRuntime.JniValueManager.CollectPeers()`? With a Mono-style GC bridge, `CollectPeers()` is `GC.Collect()`. In a666a6f with .NET Core, `CollectPeers()` calls `IJavaPeerable.Dispose()` on all registered instances, which is "extreme". @jonpryor thought that if there were a *scope-based* way to selectively control which instances were disposed, that might be "better" and more understandable. Plus, this is issue #4! Which brings us to the background for Issue #4, which touches upon [bugzilla 25443][0] and [Google issue 37034307][1]: Java.Interop attempts to provide referential identity for Java objects: if two separate Java methods invocations return the same Java instance, then the bindings of those methods should also return the same instance. This is "fine" on the surface, but has a few related issues: 1. JNI Global References are a limited resource: Android only allows ~52000 JNI Global References to exist, which sounds like a lot, but might not be. 2. Because of (1), it is not uncommon to want to use `using` blocks to invoke `IJavaPeerable.Dispose()` to release JNI Global References. 3. However, because of object identity, you could unexpectedly introduce *cross-thread sharing* of `IJavaPeerable` instances. This might not be at all obvious; for example, in the Android 5 timeframe, [`Typeface.create()`][2] wouldn't necessarily return new Java instances, but may instead return cached instances. Meaning that this: var t1 = new Thread(() => { using var typeface = Typeface.Create("Family", …); // use typeface… }); var t2 = new Thread(() => { using var typeface = Typeface.Create("Family", …); // use typeface… }); t1.Start(); t2.Start(); t1.Join(); t2.Join(); could plausibly result in `ObjectDisposedException`s (or worse), as the threads could be unexpectedly sharing the same bound instance. Which *really* means that you can't reliably call `Dispose()`, unless you *know* you created that instance: using var safe = new Java.Lang.Double(42.0); // I created this, therefore I control all access and can Dispose() it using var unsafe = Java.Lang.Double.ValueOf(42.0); // I have no idea who else may be using this instance! Attempt to address both of these scenarios -- a modicum of .NET Core support, and additional sanity around JNI Global Reference lifetimes -- by introducing a new `JavaPeerableRegistrationScope` type, which introduces a scope-based mechanism to control when `IJavaPeerable` instances are cleaned up: public enum JavaPeerableRegistrationScopeCleanup { RegisterWithManager, Dispose, Release, } public ref struct JavaPeerableRegistrationScope { public JavaPeerableRegistrationScope(JavaPeerableRegistrationScopeCleanup cleanup); public void Dispose(); } `JavaPeerableRegistrationScope` is a [`ref struct`][3], which means it can only be allocated on the runtime stack, ensuring that cleanup semantics are *scope* semantics. TODO: is that actually a good idea? If a `JavaPeerableRegistrationScope` is created using `JavaPeerableRegistrationScopeCleanup.RegisterWithManager`, existing behavior is followed. This is useful for nested scopes, should instances need to be registered with `JniRuntime.ValueManager`. If a `JavaPeerableRegistrationScope` is created using `JavaPeerableRegistrationScopeCleanup.Dispose` or `JavaPeerableRegistrationScopeCleanup.Release`, then: 1. `IJavaPeerable` instances created within the scope are "thread-local": they can be *used* by other threads, but `JniRuntime.JniValueManager.PeekPeer()` will only find the value on the creating thread. 2. At the end of a `using` block / when `JavaScope.Dispose()` is called, all collected instances will be `Dispose()`d (with `.Dispose`) or released (with `.Release`), and left to the GC to eventually finalize. For example: using (new JavaPeerableRegistrationScope (JavaPeerableRegistrationScopeCleanup.Dispose)) { var singleton = JavaSingleton.Singleton; // use singleton // If other threads attempt to access `JavaSingleton.Singleton`, // they'll create their own peer wrapper } // `singleton.Dispose()` is called at the end of the `using` block However, if e.g. the singleton instance is already accessed, then it won't be added to the registration scope and won't be disposed: var singleton = JavaSingleton.Singleton; // singleton is registered with the ValueManager // later on the same thread or some other threa… using (new JavaPeerableRegistrationScope (JavaPeerableRegistrationScopeCleanup.Dispose)) { var anotherSingleton = JavaSingleton.Singleton; // use anotherSingleton… } then `anotherSingleton` will *not* disposed, as it already existed. *Only newly created instances* are added to the current scope. By default, only `JavaPeerableRegistrationScopeCleanup.RegisterWithManager` is supported. To support the other cleanup modes, `JniRuntime.JniValueManager.SupportsPeerableRegistrationScopes` must return `true`, which in turn requires that: * `JniRuntime.JniValueManager.AddPeer()` calls `TryAddPeerToRegistrationScope()`, * `JniRuntime.JniValueManager.RemovePeer()` calls `TryRemovePeerFromRegistrationScopes()` * `JniRuntime.JniValueManager.PeekPeer()` calls `TryPeekPeerFromRegistrationScopes()`. See `ManagedValueManager` for an example implementation. Finally, add the following methods to `JniRuntime.JniValueManager` to help further assist peer management: partial class JniRuntime.JniValueManager { public virtual bool CanCollectPeers { get; } public virtual bool CanReleasePeers { get; } public virtual void ReleasePeers (); } TODO: docs? TODO: *nested* scopes, and "bound" vs. "unbound" instance construction around `JniValueManager.GetValue<T>()` or `.CreateValue<T>()`, and *why* they should be treated differently. TODO: Should `CreateValue<T>()` be *removed*? name implies it always "creates" a new value, but implementation will return existing instances, so `GetValue<T>()` alone may be better. One related difference is that` `CreateValue<T>()` uses `PeekBoxedObject()`, while `GetValue<T>()` doesn't. *Should* it? [0]: https://web.archive.org/web/20211106214514/https://bugzilla.xamarin.com/25/25443/bug.html#c63 [1]: https://issuetracker.google.com/issues/37034307 [2]: https://developer.android.com/reference/android/graphics/Typeface#create(java.lang.String,%20int) [3]: https://docs.microsoft.com/dotnet/csharp/language-reference/builtin-types/struct#ref-struct
- Loading branch information
