From e013608f03c53af62154beac634cef61f09be615 Mon Sep 17 00:00:00 2001 From: Jonathan Peppers Date: Fri, 15 Oct 2021 10:21:39 -0500 Subject: [PATCH] [tests] rework JavaObjectTest, use FinalizerHelper from mono/mono Context: https://github.com/dotnet/runtime/issues/60638#issuecomment-949849104 Context: https://github.com/xamarin/xamarin-android/pull/6363 We were seeing `JavaObjectTest.Dispose_Finalized()` fail in a .NET 6 bump 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. This did not solve this issue; we need to fix up the test to wait for two GCs to complete: FinalizerHelpers.PerformNoPinAction (() => { FinalizerHelpers.PerformNoPinAction (() => { var v = new JavaDisposedObject (() => d = true, () => f = true); GC.KeepAlive (v); }); JniEnvironment.Runtime.ValueManager.CollectPeers (); }); JniEnvironment.Runtime.ValueManager.CollectPeers (); With this change in place, the test now passes: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=5361266&view=ms.vss-test-web.build-test-results-tab We can also remove the category added in d1d64c10. --- .../Java.Interop/FinalizerHelpers.cs | 47 +++++++++++++++++++ .../Java.Interop/JavaObjectTest.cs | 28 ++++------- 2 files changed, 56 insertions(+), 19 deletions(-) create mode 100644 tests/Java.Interop-Tests/Java.Interop/FinalizerHelpers.cs diff --git a/tests/Java.Interop-Tests/Java.Interop/FinalizerHelpers.cs b/tests/Java.Interop-Tests/Java.Interop/FinalizerHelpers.cs new file mode 100644 index 000000000..26a1397b0 --- /dev/null +++ b/tests/Java.Interop-Tests/Java.Interop/FinalizerHelpers.cs @@ -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 (); + } + } +} diff --git a/tests/Java.Interop-Tests/Java.Interop/JavaObjectTest.cs b/tests/Java.Interop-Tests/Java.Interop/JavaObjectTest.cs index da38f8816..a11736061 100644 --- a/tests/Java.Interop-Tests/Java.Interop/JavaObjectTest.cs +++ b/tests/Java.Interop-Tests/Java.Interop/JavaObjectTest.cs @@ -1,5 +1,4 @@ using System; -using System.Threading; using Java.Interop; @@ -18,13 +17,11 @@ public void JavaReferencedInstanceSurvivesCollection () using (var t = new JniType ("java/lang/Object")) { var oldHandle = IntPtr.Zero; var array = new JavaObjectArray (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 (); @@ -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 (); @@ -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); @@ -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 (); }