Skip to content

Commit

Permalink
[tests] rework JavaObjectTest, use FinalizerHelper from mono/mono (#899)
Browse files Browse the repository at this point in the history
Context: dotnet/android#6363
Context: dotnet/runtime#60638 (comment)
Context: d1d64c1
Context: dotnet/android@c2c9ed4

We were seeing `JavaObjectTest.Dispose_Finalized()` fail in a .NET 6
bump in xamarin-android with `$(UseInterpreter)` set to `true`:

	Expected: True
	But was:  False
	  at Java.InteropTests.JavaObjectTest.Dispose_Finalized()
	  at System.Reflection.RuntimeMethodInfo.Invoke(Object , BindingFlags , Binder , Object[] , CultureInfo )

The first recommendation was to use a helper method from mono/mono's
unit tests:

  * https://github.com/mono/mono/blob/8266c5604b8c03882f2b06af27fdea46b142d6b9/mono/mini/TestHelpers.cs#L12

I removed usage of all `System.Threading` in `JavaObjectTest` in
favor of this helper method, which internally uses a `Thread`.

This did not solve this issue; we need to fix up the test to wait for
two GCs to complete, on two separate threads:

	FinalizerHelpers.PerformNoPinAction (() => {
	    FinalizerHelpers.PerformNoPinAction (() => {
	        var v     = new JavaDisposedObject (() => d = true, () => f = true);
	        GC.KeepAlive (v);
	    });
	    // Thread 2
	    JniEnvironment.Runtime.ValueManager.CollectPeers ();
	});
	// Thread 1
	JniEnvironment.Runtime.ValueManager.CollectPeers ();

`ValueManager.CollectPeers()` uses `GC.Collect()`, and `GC.Collect()`
needs to be called from two separate threads because, as per @BrzVlad:

> if an object is alive then the GC will scan it and, by doing so, it
> will probably store it in the stack somewhere.  In very unfortunate
> scenarios, at the second collection, the pinning step will find this
> reference existing on the stack and will pin the object (that might
> have now been dead otherwise).  Because the `JavaDisposedObject` is
> alive during the first collection, we do this collection on a
> separate thread, so the second GC doesn't get to find this left over
> references on the stack. 

With this change in place, the test now passes.

We can also remove the category added in d1d64c1.
  • Loading branch information
jonathanpeppers committed Oct 26, 2021
1 parent f658ab2 commit 220b87f
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 19 deletions.
47 changes: 47 additions & 0 deletions tests/Java.Interop-Tests/Java.Interop/FinalizerHelpers.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// Originally from: https://github.com/mono/mono/blob/8266c5604b8c03882f2b06af27fdea46b142d6b9/mono/mini/TestHelpers.cs#L12

using System;
using System.Threading;

namespace Java.InteropTests
{
// False pinning cases are still possible. For example the thread can die
// and its stack reused by another thread. It also seems that a thread that
// does a GC can keep on the stack references to objects it encountered
// during the collection which are never released afterwards. This would
// be more likely to happen with the interpreter which reuses more stack.
static class FinalizerHelpers
{
private static IntPtr aptr;

private static unsafe void NoPinActionHelper (int depth, Action act)
{
// Avoid tail calls
int* values = stackalloc int [20];
aptr = new IntPtr (values);

if (depth <= 0) {
//
// When the action is called, this new thread might have not allocated
// anything yet in the nursery. This means that the address of the first
// object that would be allocated would be at the start of the tlab and
// implicitly the end of the previous tlab (address which can be in use
// when allocating on another thread, at checking if an object fits in
// this other tlab). We allocate a new dummy object to avoid this type
// of false pinning for most common cases.
//
new object ();
act ();
} else {
NoPinActionHelper (depth - 1, act);
}
}

public static void PerformNoPinAction (Action act)
{
Thread thr = new Thread (() => NoPinActionHelper (128, act));
thr.Start ();
thr.Join ();
}
}
}
28 changes: 9 additions & 19 deletions tests/Java.Interop-Tests/Java.Interop/JavaObjectTest.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
using System;
using System.Threading;

using Java.Interop;

Expand All @@ -18,13 +17,11 @@ public void JavaReferencedInstanceSurvivesCollection ()
using (var t = new JniType ("java/lang/Object")) {
var oldHandle = IntPtr.Zero;
var array = new JavaObjectArray<JavaObject> (1);
var w = new Thread (() => {
FinalizerHelpers.PerformNoPinAction (() => {
var v = new JavaObject ();
oldHandle = v.PeerReference.Handle;
array [0] = v;
});
w.Start ();
w.Join ();
JniEnvironment.Runtime.ValueManager.CollectPeers ();
GC.WaitForPendingFinalizers ();
GC.WaitForPendingFinalizers ();
Expand Down Expand Up @@ -80,13 +77,11 @@ public void UnreferencedInstanceIsCollected ()
{
JniObjectReference oldHandle = new JniObjectReference ();
WeakReference r = null;
var t = new Thread (() => {
FinalizerHelpers.PerformNoPinAction (() => {
var v = new JavaObject ();
oldHandle = v.PeerReference.NewWeakGlobalRef ();
r = new WeakReference (v);
});
t.Start ();
t.Join ();
JniEnvironment.Runtime.ValueManager.CollectPeers ();
GC.WaitForPendingFinalizers ();
GC.WaitForPendingFinalizers ();
Expand All @@ -110,20 +105,17 @@ public void Dispose ()

#if !NO_GC_BRIDGE_SUPPORT
[Test]
// See: https://github.com/dotnet/runtime/issues/60638
[Category ("IgnoreInterpreter")]
public void Dispose_Finalized ()
{
var d = false;
var f = false;
var t = new Thread (() => {
var v = new JavaDisposedObject (() => d = true, () => f = true);
GC.KeepAlive (v);
FinalizerHelpers.PerformNoPinAction (() => {
FinalizerHelpers.PerformNoPinAction (() => {
var v = new JavaDisposedObject (() => d = true, () => f = true);
GC.KeepAlive (v);
});
JniEnvironment.Runtime.ValueManager.CollectPeers ();
});
t.Start ();
t.Join ();
JniEnvironment.Runtime.ValueManager.CollectPeers ();
GC.WaitForPendingFinalizers ();
JniEnvironment.Runtime.ValueManager.CollectPeers ();
GC.WaitForPendingFinalizers ();
Assert.IsFalse (d);
Expand Down Expand Up @@ -185,11 +177,9 @@ public void Ctor_Exceptions ()
public void CrossThreadSharingRequresRegistration ()
{
JavaObject o = null;
var t = new Thread (() => {
FinalizerHelpers.PerformNoPinAction (() => {
o = new JavaObject ();
});
t.Start ();
t.Join ();
o.ToString ();
o.Dispose ();
}
Expand Down

0 comments on commit 220b87f

Please sign in to comment.