[Java.Interop] Add JavaPeerableRegistrationScope

[jonpryor]

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 and Google issue 37034307: 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() 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 ObjectDisposedExceptions (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, 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()usesPeekBoxedObject(), while GetValue()` doesn't. Should it?

[jpobst]

1. Can you elaborate on why threading matters here? For example, using the Typeface.Create sample above, what happens if threading is removed? Is this ok?

var typeface1 = Typeface.Create("Family", …);
var typeface2 = Typeface.Create("Family", …);

typeface1.Dispose ();

Console.WriteLine (typeface2.Weight);

2. Is scoping a better/more feasible solution than an automatic reference counting solution? It feels like today's implementation is a hidden leaky abstraction where users will never know what is happening, much less discover how to fix it using this proposal.

Ideally it would "just work" without additional knowledge or effort, but perhaps we just can't get there from where we are today?

[jonpryor]

@jpobst asked:

1. Can you elaborate on why threading matters here?

You're correct that threading doesn't per-se matter here, it's just that threading increases the "spooky action at a distance" meme. Sheer code size can also increase "spooky action at a distance".

is this okay?

var typeface1 = Typeface.Create("sans-serif", TypefaceStyle.Normal);
var typeface2 = Typeface.Create("sans-serif", TypefaceStyle.Normal);

typeface1.Dispose ();

Console.WriteLine (typeface2.Weight);

It is in fact not okay, for the reason I attempted to explain in the commit message: even though the method name says "create", it does not in fact necessarily create a new instance!

In this case, typeface1 and typeface2 are the same instance!

Console.WriteLine ($"# jonp: typeface1={typeface1.PeerReference}, typeface2={typeface2.PeerReference}");
Console.WriteLine ($"# jonp: typeface1==typeface2: {typeface1 == typeface2}");

prints to logcat:

# jonp: typeface1=0x39f2/G, typeface2=0x39f2/G
# jonp: typeface1==typeface2: True

which in turn means that typeface1.Dispose() also disposes typeface2, because they refer to the same object!

Which in turn means that typeface2.Weight results in an ObjectDisposedException:

E AndroidRuntime: android.runtime.JavaProxyThrowable: [System.ObjectDisposedException]: Cannot access a disposed object.
E AndroidRuntime: Object name: 'Android.Graphics.Typeface'.
E AndroidRuntime:        at Java.Interop.JniPeerMembers.AssertSelf(Unknown Source:0)
E AndroidRuntime:        at Java.Interop.JniPeerMembers+JniInstanceMethods.InvokeVirtualInt32Method(Unknown Source:0)
E AndroidRuntime:        at Android.Graphics.Typeface.get_Weight(Unknown Source:0)
E AndroidRuntime:        at android_Typeface.create.MainActivity.OnCreate(Unknown Source:0)
E AndroidRuntime:        at Android.App.Activity.n_OnCreate_Landroid_os_Bundle_(Unknown Source:0)
E AndroidRuntime:        at Android.Runtime.JNINativeWrapper.Wrap_JniMarshal_PPL_V(Unknown Source:0)
E AndroidRuntime:        at crc64279dedc6a0a409e6.MainActivity.n_onCreate(Native Method)
E AndroidRuntime:        at crc64279dedc6a0a409e6.MainActivity.onCreate(MainActivity.java:30)

As they say, "Boom goes the dynamite!"

2. Is scoping a better/more feasible solution than an automatic reference counting solution.

While #426 *says "Reference Counting, non-bridged backend", I've already mentally dropped the "reference counting" bit as it doesn't really make sense, as there's no way in normal C# use to automatically increment or decrement a reference count. Instead, what we already have is a peer has a "ref count" of 1 or 0: it has a JniObjectReference ("ref count = 1"), or it doesn't ("ref count = 0"). Within that world order, the same peer instance can be referenced from multiple locations, while the "ref count" would remain 1 until it is disposed.

This is what we see in the preceding Typeface.Create() code: there's one peer, with two variables referencing it.

Which is to say, I cannot conceive of what an "automatic reference counting solution" means or looks like while retaining existing JniRuntime.JniValueManager semantics and the current "feel" of our bindings.

[jpobst]

there's no way in normal C# use to automatically increment or decrement a reference count

I probably used the wrong terminology to describe what I was thinking.

All peerable wrapper creation is presumably centralized to one location (GetValue<T>?), which either creates a new wrapper (new Double (42.0)) or returns an existing wrapper if it exists (Double.ValueOf (42.0)).

Similarly, all wrapper destruction is centralized to IJavaPeerable.Dispose ().

Could we store a "reference count" in the wrapper that denotes how many times we have returned the wrapper via GetValue<T>? Additionally, modify IJavaPeerable.Dispose () to decrement the count and only dispose the wrapper when it hits zero.

For example:

var typeface1 = Typeface.Create ("Family", …);  // Returns new wrapper with "ref count" of 1
var typeface2 = Typeface.Create ("Family", …);  // Returns same wrapper with "ref count" incremented to 2

typeface1.Dispose ();   // Wrapper "ref count" decremented to 1, wrapper not disposed
Console.WriteLine (typeface2.Weight);   // This succeeds because the wrapper still exists

typeface2.Dispose ();   // Wrapper "ref count" decremented to 0, wrapper is disposed
Console.WriteLine (typeface2.Weight);   // This fails because the wrapper is disposed

This would not be the "true" "reference counting" which isn't easily supportable by C#. That is, the following would still fail, but the failure would be consistent with all other object instances in C#.

var typeface1 = Typeface.Create ("Family", …);  // Returns new wrapper with "ref count" of 1
var typeface2 = typeface1;

typeface1.Dispose ();   // Wrapper "ref count" decremented to 0, wrapper is disposed
Console.WriteLine (typeface2.Weight);   // This fails because the wrapper is disposed

[jonpryor]

@jpobst wrote:

Could we store a "reference count" in the wrapper that denotes how many times we have returned the wrapper via GetValue<T>? Additionally, modify IJavaPeerable.Dispose () to decrement the count and only dispose the wrapper when it hits zero.

This is an idea that hadn't occurred to me, and I'm not at all sure that I like it. For starters, it means that IDisposable.Dispose() is no longer idempotent.

I also think that this won't actually work the way we think it will, due to properties that return collections. Consider the BufferedInputStream.Buf property:

namespace Java.IO {

	public partial class BufferedInputStream : Java.IO.FilterInputStream {
		// .NET for Android
		protected IList<byte>? Buf {
			get {
				const string __id = "buf.[B";

				var __v = _members.InstanceFields.GetObjectValue (__id, this);
				return global::Android.Runtime.JavaArray<byte>.FromJniHandle (__v.Handle, JniHandleOwnership.TransferLocalRef);
		}

		// Java.Base
		protected global::Java.Interop.JavaSByteArray? Buf {
			get {
				const string __id = "buf.[B";

				var __v = _members.InstanceFields.GetObjectValue (__id, this);
				return global::Java.Interop.JniEnvironment.Runtime.ValueManager.GetValue<global::Java.Interop.JavaSByteArray? >(ref __v, JniObjectReferenceOptions.Copy);
			}
		}
	}
}

Next, consider usage of this property:

class Wat : Java.IO.BufferedInputStream
{
    public Wat(System.IO.Stream? @in, int size) : base (@in, size)
    {
    }

    void Hm()
    {
        for (int i =0; i < Buf.Count; ++i) {
            Console.WriteLine (Buf[i]);
        }
    }
}

What does the repeated access of the Buf property look like? After compiling and disassembling it's something like:

    .locals init (int32 V_0,
             bool V_1)
    IL_0000:  nop
    IL_0001:  ldc.i4.0
    IL_0002:  stloc.0
    IL_0003:  br.s       IL_001d

    IL_0005:  nop
    IL_0006:  ldarg.0
    IL_0007:  call       instance [System.Runtime]System.Collections.Generic.IList`1<uint8> [Mono.Android]Java.IO.BufferedInputStream::get_Buf()
    IL_000c:  ldloc.0
    IL_000d:  callvirt   instance !0 [System.Runtime]System.Collections.Generic.IList`1<uint8>::get_Item(int32)
    IL_0012:  call       void [System.Console]System.Console::WriteLine(int32)
    IL_0017:  nop
    IL_0018:  nop
    IL_0019:  ldloc.0
    IL_001a:  ldc.i4.1
    IL_001b:  add
    IL_001c:  stloc.0
    IL_001d:  ldloc.0
    IL_001e:  ldarg.0
    IL_001f:  call       instance [System.Runtime]System.Collections.Generic.IList`1<uint8> [Mono.Android]Java.IO.BufferedInputStream::get_Buf()
    IL_0024:  callvirt   instance int32 [System.Runtime]System.Collections.Generic.ICollection`1<uint8>::get_Count()
    IL_0029:  clt
    IL_002b:  stloc.1
    IL_002c:  ldloc.1
    IL_002d:  brtrue.s   IL_0005

    IL_002f:  ret

Notice there are two get_Buf() invocations, at IL_0007 and IL_001f. Building the same Wat subclass against Java.Base results in virtually identical IL, except instead of get_Buf() returning a [System.Runtime]System.Collections.Generic.IList`1<uint8>, it returns a [Java.Interop]Java.Interop.JavaSByteArray.

With a "GetValue<T>() bumps the ref count" idea and when properties invoke GetValue<T>() -- which is the case for "desktop-style bindings" -- then the ref count associated with with the value returned by Buf will constantly increase. This approach might work on .NET for Android, as collection-like properties don't use GetValue<T>, but instead e.g. JavaArray<T>.FromJniHandle(), which uses PeekObject() instead of GetValue<T>(). (Though FromJniHandle() could be updated in a manner similar to that suggested for GetValue<T>(), which would have the same "constantly increasing refcount" problem.)

Additionally, updating things so that your proposed code works:

var typeface1 = Typeface.Create ("Family", …);  // Returns new wrapper with "ref count" of 1
var typeface2 = Typeface.Create ("Family", …);  // Returns same wrapper with "ref count" incremented to 2

typeface1.Dispose ();   // Wrapper "ref count" decremented to 1, wrapper not disposed
Console.WriteLine (typeface2.Weight);   // This succeeds because the wrapper still exists

makes my brain hurt: on the one hand. On .NET for Android, would the refcount be considered by the GC bridge code, i.e. if we hit the GC bridge and an instance has a non-zero refcount, will the instance still be eligible for collection? If it will still be eligible for collection, then the refcount becomes "weird" ("my object has a refcount of 2, yet it was collected!"). If it won't be eligible for collection, then I don't want to imagine how many memory leaks this will plausibly introduce, as .Dispose() now becomes required.

This might be plausible for Desktop bindings, but I want abstractions that abstract over CoreCLR & MonoVM, and this feels like a hugely leaky abstraction.

[jonpryor]

While discussing this with @jonathanpeppers, one "foot gun" with this idea is that you can't use it "indiscriminately". Consider Activity.OnCreate(), and then you, as the app author, think "I want to use this new feature!", and do:

partial class Activity {
    public override void OnCreate(Bundle? savedInstanceState)
    {
        using (new JavaPeerableRegistrationScope (JavaPeerableRegistrationScopeCleanup.Dispose)) {
            base.OnCreate(savedInstanceState);
            Xamarin.Essentials.Platform.Init(this, savedInstanceState);
            SetContentView(Resource.Layout.activity_main);

            Android.Support.V7.Widget.Toolbar toolbar = FindViewById<Android.Support.V7.Widget.Toolbar>(Resource.Id.toolbar);
            SetSupportActionBar(toolbar);

            FloatingActionButton fab = FindViewById<FloatingActionButton>(Resource.Id.fab);
            fab.Click += FabOnClick;
        }
    }
}

What will happen is that things will almost certainly not work. In particular, FindViewById<T>() will return a "thread-local value", fab.Click += FabOnClick will update that thread-local value, and the implicit Dispose() at the end of the using block will dispose fab. (It'll also dispose toolbar, and $DEITY knows what it'll do to Xamarin.Essentials.Platform.Init(…)…)

Some of that could actually be useful: toolbar can very likely be disposed (unless it's a C# subclass within the .axml file), so cleaning up toolbar might be nice! However, fab must not be disposed.

That's potentially obvious here, but if the fab.Click event subscription is called N stack frames away from the using (new JavaPeerableRegistrationScope) block, that'll be some very spooky action at distance.

[jonpryor]

One idea that comes to mind to address the above foot gun comment would be to cache recently used JniIdentityHashCode values and log if we fail to lookup a recently observed JniIdentityHashCode value.

[jonpryor]

Yet another fundamental problem with this idea: caching.

Issue #1243 proposes caching all final fields:

			global::Java.Lang.Thread.State? _Runnable_cache;
			public static global::Java.Lang.Thread.State? Runnable {
				get {
					if (_Runnable_cache != null) return _Runnable_cache;
					const string __id = "RUNNABLE.Ljava/lang/Thread$State;";

					var __v = _members.StaticFields.GetObjectValue (__id);
					return _Runnable_cache  = global::Java.Interop.JniEnvironment.Runtime.ValueManager.GetValue<global::Java.Lang.Thread.State? >(ref __v, JniObjectReferenceOptions.Copy);
				}
			}

Now, imagine such caching intermixed with JavaPeerableRegistrationScope:

using (new JavaPeerableRegistrationScope(JavaPeerableRegistrationScopeCleanup.Dispose)) {
    var r = Java.Lang.Thread.State.Runnable;
}

Accessing Thread.State.Runnable would cause Thread.State._Runnable_cache to hold a cached value, which is then disposed at the end of the using block. Subsequently, accessing Java.Lang.Thread.State.Runnable from anywhere will return an already disposed value.

Additionally, such caching is invisible and unknowable, at any level of the stack. (See also: dotnet/maui#24248 )

Which feels like a final nail against this idea.

[jonpryor]

Closing, as I don't see a way to preserve this idea that wouldn't break other things; see #1238 (comment).