Skip to content

Conversation

@jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Oct 22, 2021

Context: dotnet/runtime#60638 (comment)
Context: dotnet/android#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 d1d64c1.

jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this pull request Oct 22, 2021
@jonathanpeppers jonathanpeppers marked this pull request as draft October 22, 2021 19:26
Context: dotnet/runtime#60638 (comment)
Context: dotnet/android#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 d1d64c1.
@jonathanpeppers jonathanpeppers changed the title [tests] use FinalizerHelper from mono/mono [tests] rework JavaObjectTest, use FinalizerHelper from mono/mono Oct 22, 2021
@jonathanpeppers jonathanpeppers marked this pull request as ready for review October 22, 2021 21:45
// be more likely to happen with the interpreter which reuses more stack.
static class FinalizerHelpers
{
private static IntPtr aptr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BrzVlad: Why is this member needed at all?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Answer: probably to prevent optimizing out the stackalloc: https://discord.com/channels/732297728826277939/732297837953679412/902261731471007765

var v = new JavaDisposedObject (() => d = true, () => f = true);
GC.KeepAlive (v);
});
JniEnvironment.Runtime.ValueManager.CollectPeers ();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change somewhat confuses me: we need to call GC.Collect() from both a new thread and the main thread in order to get the test to pass? Instead of invoking GC.Collect() from the main thread twice?

This is very confusing to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I understood a single call to FinalizerHelpers.PerformNoPinAction() ensures one GC.

And so we need two GCs for the finalizer to always run in this case. @BrzVlad is that correct?

Details here: dotnet/runtime#60638 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that, 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.

@jonpryor
Copy link
Contributor

jonpryor commented Oct 26, 2021

work-in-progress commit message:

Context: https://github.com/xamarin/xamarin-android/pull/6363
Context: https://github.com/dotnet/runtime/issues/60638#issuecomment-949849104
Context: d1d64c10281eaa895fa415dfcafb8ed716a0db3c
Context: https://github.com/xamarin/xamarin-android/commit/c2c9ed47c94a03801986233ec9db860f0ef1daca

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.

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.

@jonpryor jonpryor merged commit 220b87f into main Oct 26, 2021
@jonpryor jonpryor deleted the finalizerhelper branch October 26, 2021 16:58
@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants