Skip to content

Commit

Permalink
[Java.Interop] Add JniPeerRegistrationScope?
Browse files Browse the repository at this point in the history
Fixes: dotnet#4

Context: dotnet#426

Alternate names?

  * JavaPeerableCleanupPool
  * JavaPeerCleanup
  * JavaReferenceCleanup

Issue dotnet#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 do 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 dotnet#4!

Add `JniPeerRegistrationScope`, which introduces a scope-based
mechanism to control when `IJavaPeerable` instances are cleaned up:

	public enum JniPeerRegistrationScopeCleanup {
	    RegisterWithManager,
	    Dispose,
	    Release,
	}

	public ref struct JniPeerRegistrationScope {
	    public JniPeerRegistrationScope(JniPeerRegistrationScopeCleanup cleanup);
	    public void Dispose();
	}

`JniPeerRegistrationScope` is a [`ref struct`][0], 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 `JniPeerRegistrationScope` is created using
`JniPeerRegistrationScopeCleanup.RegisterWithManager`, existing
behavior is followed.  This is useful for nested scopes, should
instances need to be registered with `JniRuntime.ValueManager`.

If a `JniPeerRegistrationScope` is created using `.Dispose` or
`.Release`, then:

 1. Object references created within the scope are "thread-local";
    they can be *used* by other threads, but
    `JniRuntime.JniValueManager.PeekPeer()` won't find existing values.

 2. At the end of a `using` block / when
   `JniPeerRegistrationScope.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 JniPeerRegistrationScope (JniPeerRegistrationScopeCleanup.Dispose)) {
	    var singleton = JavaSingleton.Singleton;
	    // use singleton
	}
	// `singleton.Dispose()` is called at the end of the `using` block

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://docs.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/struct#ref-struct
  • Loading branch information
jonpryor committed May 5, 2021
1 parent f4e68b5 commit bf28aef
Show file tree
Hide file tree
Showing 16 changed files with 925 additions and 238 deletions.
2 changes: 2 additions & 0 deletions src/Java.Interop/GlobalSuppressions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

[assembly: SuppressMessage ("Design", "CA1024:Use properties where appropriate", Justification = "<Pending>", Scope = "member", Target = "~M:Java.Interop.JniRuntime.GetRegisteredRuntimes()")]

[assembly: SuppressMessage ("Design", "CA1031:Do not catch general exception types", Justification = "Excceptions are bundled into an AggregateException and rethrown", Scope = "type", Target = "~M:Java.Interop.JniPeerRegistrationScope.Dispose")]

[assembly: SuppressMessage ("Design", "CA1032:Implement standard exception constructors", Justification = "System.Runtime.Serialization.SerializationInfo doesn't exist in our targeted PCL profile, so we can't provide the (SerializationInfo, StreamingContext) constructor.", Scope = "type", Target = "~T:Java.Interop.JavaProxyThrowable")]
[assembly: SuppressMessage ("Design", "CA1032:Implement standard exception constructors", Justification = "System.Runtime.Serialization.SerializationInfo doesn't exist in our targeted PCL profile, so we can't provide the (SerializationInfo, StreamingContext) constructor.", Scope = "type", Target = "~T:Java.Interop.JniLocationException")]

Expand Down
20 changes: 6 additions & 14 deletions src/Java.Interop/Java.Interop/JavaProxyObject.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#nullable enable

using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;

Expand Down Expand Up @@ -28,10 +29,8 @@ public override JniPeerMembers JniPeerMembers {
}
}

JavaProxyObject (object value)
internal JavaProxyObject (object value)
{
if (value == null)
throw new ArgumentNullException (nameof (value));
Value = value;
}

Expand All @@ -56,18 +55,11 @@ public override bool Equals (object? obj)
return Value.ToString ();
}

[return: NotNullIfNotNull ("object")]
public static JavaProxyObject? GetProxy (object value)
protected override void Dispose (bool disposing)
{
if (value == null)
return null;

lock (CachedValues) {
if (CachedValues.TryGetValue (value, out var proxy))
return proxy;
proxy = new JavaProxyObject (value);
CachedValues.Add (value, proxy);
return proxy;
base.Dispose (disposing);
if (disposing) {
CachedValues.Remove (Value);
}
}

Expand Down
71 changes: 71 additions & 0 deletions src/Java.Interop/Java.Interop/JniEnvironment.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@

using System;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Diagnostics;
using System.Linq;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Text;
using System.Threading;

namespace Java.Interop {
Expand Down Expand Up @@ -189,6 +191,8 @@ sealed class JniEnvironmentInfo : IDisposable {
bool disposed;
JniRuntime? runtime;

List<PeerableCollection>? scopes;

public int LocalReferenceCount {get; internal set;}
public bool WithinNewObjectScope {get; set;}
public JniRuntime Runtime {
Expand Down Expand Up @@ -229,6 +233,12 @@ public bool IsValid {
get {return Runtime != null && environmentPointer != IntPtr.Zero;}
}

public List<PeerableCollection>?
Scopes => scopes;

public PeerableCollection? CurrentScope =>
scopes == null ? null : scopes [scopes.Count-1];

public JniEnvironmentInfo ()
{
Runtime = JniRuntime.CurrentRuntime;
Expand Down Expand Up @@ -279,6 +289,29 @@ public void Dispose ()
disposed = true;
}

public PeerableCollection BeginScope (JniPeerRegistrationScopeCleanup cleanup)
{
scopes = scopes ?? new List<PeerableCollection?> ();
var scope = new PeerableCollection () {
Cleanup = cleanup,
};
scopes.Add (scope);
return scope;
}

public void EndScope (PeerableCollection scope)
{
Debug.Assert (scopes != null);
if (scopes == null) {
return;
}
Debug.Assert (scopes.Count > 0);
scopes.Remove (scope);
if (scopes.Count == 0) {
scopes = null;
}
}

#if FEATURE_JNIENVIRONMENT_SAFEHANDLES
internal List<List<JniLocalReference>> LocalReferences = new List<List<JniLocalReference>> () {
new List<JniLocalReference> (),
Expand All @@ -295,5 +328,43 @@ static unsafe JniEnvironmentInvoker CreateInvoker (IntPtr handle)
}
#endif // !FEATURE_JNIENVIRONMENT_JI_PINVOKES
}

class PeerableCollection : KeyedCollection<int, IJavaPeerable> {

public JniPeerRegistrationScopeCleanup Cleanup { get; set; }

protected override int GetKeyForItem (IJavaPeerable item) => item.JniIdentityHashCode;

public IJavaPeerable? GetPeerableForObjectReference (JniObjectReference reference)
{
#if NETCOREAPP
if (TryGetValue (JniSystem.IdentityHashCode (reference), out var p) &&
JniEnvironment.Types.IsSameObject (reference, p.PeerReference)) {
return p;
}
#else // !NETCOREAPP
Collection<IJavaPeerable> c = this;
for (int x = 0; x < c.Count; ++x) {
if (JniEnvironment.Types.IsSameObject (reference, c [x].PeerReference)) {
return c [x];
}
}
#endif // !NETCOREAPP
return null;
}

public override string ToString ()
{
var c = (Collection<IJavaPeerable>) this;
var s = new StringBuilder ();
s.Append ("PeerableCollection[").Append (Count).Append ("]");
for (int i = 0; i < Count; ++i ) {
s.AppendLine ();
var e = c [i];
s.Append ($" [{i}] hash={e.JniIdentityHashCode} ref={e.PeerReference} type={e.GetType ().ToString ()} value=`{e.ToString ()}`");
}
return s.ToString ();
}
}
}

55 changes: 55 additions & 0 deletions src/Java.Interop/Java.Interop/JniPeerRegistrationScope.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;

namespace Java.Interop {

public enum JniPeerRegistrationScopeCleanup {
RegisterWithManager,
Dispose,
Release,
}

public ref struct JniPeerRegistrationScope {

JniPeerRegistrationScopeCleanup? cleanup;
PeerableCollection? scope;

public JniPeerRegistrationScope (JniPeerRegistrationScopeCleanup cleanup)
{
this.cleanup = cleanup;
scope = JniEnvironment.CurrentInfo.BeginScope (cleanup);
}

public void Dispose ()
{
if (cleanup == null || scope == null) {
return;
}
List<Exception>? exceptions = null;
switch (cleanup) {
case JniPeerRegistrationScopeCleanup.Dispose:
// Need to iterate over a copy of `scope`, as `p.Dispose()` will modify `scope`
var copy = new IJavaPeerable [scope.Count];
scope.CopyTo (copy, 0);
foreach (var p in copy) {
try {
p.Dispose ();
}
catch (Exception e) {
exceptions = exceptions ?? new List<Exception>();
exceptions.Add (e);
Trace.WriteLine (e);
}
}
break;
}
JniEnvironment.CurrentInfo.EndScope (scope);
scope.Clear ();
scope = null;
if (exceptions != null) {
throw new AggregateException (exceptions);
}
}
}
}
Loading

0 comments on commit bf28aef

Please sign in to comment.