Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[main] Update dependencies from dotnet/installer #6363

Merged
merged 12 commits into from
Oct 20, 2021

Conversation

dotnet-maestro[bot]
Copy link
Contributor

@dotnet-maestro dotnet-maestro bot commented Oct 4, 2021

This pull request updates the following dependencies

Coherency Updates

The following updates ensure that dependencies with a CoherentParentDependency
attribute were produced in a build used as input to the parent dependency's build.
See Dependency Description Format

  • Coherency Updates:
    • Microsoft.NETCore.App.Ref: from 6.0.0-rtm.21472.13 to 6.0.0-rtm.21517.2 (parent: Microsoft.Dotnet.Sdk.Internal)

From https://github.com/dotnet/installer

  • Subscription: 6548876b-06a1-4ab6-a5a5-08d8ed868088
  • Build: 20211018.6
  • Date Produced: October 18, 2021 11:40:38 AM UTC
  • Commit: 4cba5995022e769f2f9688ce44d8fc204559e4d9
  • Branch: refs/heads/release/6.0.1xx

…210930.21

Microsoft.Dotnet.Sdk.Internal
 From Version 6.0.100-rtm.21476.2 -> To Version 6.0.100-rtm.21480.21

Dependency coherency updates

Microsoft.NETCore.App.Ref
 From Version 6.0.0-rtm.21472.13 -> To Version 6.0.0-rtm.21479.7 (parent: Microsoft.Dotnet.Sdk.Internal
@jonpryor
Copy link
Member

jonpryor commented Oct 7, 2021

The remaining failure is in Java.InteropTests.JavaObjectTest.Dispose_Finalized:

Expected: True
But was:  False

   at Java.InteropTests.JavaObjectTest.Dispose_Finalized()
   at System.Reflection.RuntimeMethodInfo.Invoke(Object , BindingFlags , Binder , Object[] , CultureInfo )

The test:

The failing assert is Assert.IsTrue (f). The test expects the JavaDisposedObject finalizer to be executed; when the finalizer is executed, JavaDisposedObject.Dispose(false) will be invoked, which will invoke JavaDisposedObject.OnFinalized(), which sets f to true. The assertion fails because f is false, presumably because JavaDisposedObject.OnFinalized() isn't invoked, presumably because .Dispose(false) isn't invoked, presumably because the finalizer isn't invoked.

Has the .NET 6 interpreter changed GC finalizer semantics?

dotnet/runtime changes: http://github.com/dotnet/runtime/compare/491ed9a112559872c31ae40da8f6d9977ba4ce3a...e013f14c5330375a18656dea8dca14e5602a3ae0

@BrzVlad
Copy link
Member

BrzVlad commented Oct 7, 2021

@jonpryor Nothing changed. The failure is likely just because it is hard to guarantee object finalization with mono. The best way to deterministically finalize objects, that was proven to work fine, is https://github.com/mono/mono/blob/main/mono/mini/TestHelpers.cs#L12

@jonpryor
Copy link
Member

jonpryor commented Oct 8, 2021

@BrzVlad : then what are we doing "wrong"? https://github.com/xamarin/java.interop/blob/4277ac96eb4f2dd191e86f7bc1b72f7bdd4fbb0c/tests/Java.Interop-Tests/Java.Interop/JavaObjectTest.cs#L112-L129

We're allocating the "to-be-finalized" instance on its own Thread, and running GC.Collect(); GC.WaitForPendingFinalizers() twice, and the instance isn't being collected.

Do we need to add a new object() to the Thread to address:

				// 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.

@BrzVlad
Copy link
Member

BrzVlad commented Oct 8, 2021

@jonpryor It's either that or that the object ref is not stored deep enough on the call stack. I recommend you just copy that method in some shared testing library / sources, and then you use FinalizerHelpers.PerformNoPinAction in all tests that require waiting for finalization. As we did in mono/mono.

…211008.19

Microsoft.Dotnet.Sdk.Internal
 From Version 6.0.100-rtm.21476.2 -> To Version 6.0.100-rtm.21508.19

Dependency coherency updates

Microsoft.NETCore.App.Ref
 From Version 6.0.0-rtm.21472.13 -> To Version 6.0.0-rtm.21508.5 (parent: Microsoft.Dotnet.Sdk.Internal
@jonathanpeppers
Copy link
Member

@BrzVlad this test also started failing on this bump for the private RC 2 builds:

https://github.com/xamarin/xamarin-android-private/pull/1

I believe this test has been passing for all .NET 6 development until now. Do you know why it started failing?

jonathanpeppers added a commit to jonathanpeppers/java.interop that referenced this pull request Oct 15, 2021
Context: dotnet/android#6363

We're seeing a test failure in the latest .NET 6 bump:

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

The 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.
@jonathanpeppers jonathanpeppers marked this pull request as draft October 15, 2021 15:38
@jonathanpeppers
Copy link
Member

Got sidetracked with the Hackathon, but I pushed a commit to see if FinalizerHelper fixes this.

@jonathanpeppers jonathanpeppers force-pushed the darc-main-14333f32-1f73-4599-8639-ac5d3fcbe79d branch from 15585ab to d91fe45 Compare October 15, 2021 15:44
@jonpryor
Copy link
Member

jonpryor commented Oct 16, 2021

Looks like it's still busted, even after adding that FinalizeHelper change to Java.Interop: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=5334115&view=ms.vss-test-web.build-test-results-tab&runId=27275994&resultId=100383&paneView=debug

I think "something" actually broke in this bump.

…211018.6

Microsoft.Dotnet.Sdk.Internal
 From Version 6.0.100-rtm.21476.2 -> To Version 6.0.100-rtm.21518.6

Dependency coherency updates

Microsoft.NETCore.App.Ref
 From Version 6.0.0-rtm.21472.13 -> To Version 6.0.0-rtm.21517.2 (parent: Microsoft.Dotnet.Sdk.Internal
@jonathanpeppers
Copy link
Member

jonathanpeppers commented Oct 18, 2021

@BrzVlad I could repro this with:

https://github.com/jonathanpeppers/net6-finalizer-repro

It is interesting that it worked fine with one call, and breaks when I used Parallel.For (0, 100, ...):

jonathanpeppers/net6-finalizer-repro@a2fd388

@jonathanpeppers
Copy link
Member

Ok, I refine the repro some more.

It seems to happen with a for-loop, and you don't need to use the interpreter:

https://github.com/jonathanpeppers/net6-finalizer-repro

I added a UpdateMonoRuntimePacks you can downgrade the Mono runtime packs and see the issue goes away.

jonpryor pushed a commit to dotnet/java-interop that referenced this pull request Oct 20, 2021
)

Context: dotnet/android#6363
Context: dotnet/android#6363 (comment)
Context: #893
Context: dotnet/runtime#60638
Context: dotnet/runtime@491ed9a...e013f14

An "odd" problem arose in dotnet/android#6363, which
attempted to bump to dotnet/runtime@e013f14c:
the [`JavaObjectTest.Dispose_Finalized()` test][0] started failing.
Doubly odd is that this test has been working fine for *years*, and
there are no "obviously relevant" changes in the dotnet/runtime diff.

There was a suggestion to use try using
[`FinalizeHelpers.PerformNoPinAction()`][1], but this didn't fix the
failing unit test.

In order to "unblock" the dotnet/runtime bump in
xamarin/xamarin-android, add a `[Category ("IgnoreInterpreter")]` to
the `Dispose_Finalized()` test, so that it can be ignored for now.

We have also filed dotnet/runtime#60638 to track a proper fix.

[0]: https://github.com/xamarin/java.interop/blob/4277ac96eb4f2dd191e86f7bc1b72f7bdd4fbb0c/tests/Java.Interop-Tests/Java.Interop/JavaObjectTest.cs#L112-L129
[1]: https://github.com/mono/mono/blob/8266c5604b8c03882f2b06af27fdea46b142d6b9/mono/mini/TestHelpers.cs#L12-L44
Changes: dotnet/java-interop@2d5431f...d1d64c1

[tests] add [Category("IgnoreInterpreter")] to Dispose_Finalized()
@jonathanpeppers jonathanpeppers marked this pull request as ready for review October 20, 2021 20:00
@jonpryor jonpryor merged commit c2c9ed4 into main Oct 20, 2021
@jonpryor jonpryor deleted the darc-main-14333f32-1f73-4599-8639-ac5d3fcbe79d branch October 20, 2021 21:57
jonathanpeppers added a commit to dotnet/java-interop that referenced this pull request Oct 22, 2021
Context: dotnet/android#6363

We're seeing a test failure in the latest .NET 6 bump:

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

The 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.
jonathanpeppers added a commit to dotnet/java-interop that referenced this pull request 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.
jonpryor pushed a commit to dotnet/java-interop that referenced this pull request Oct 26, 2021
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.
@github-actions github-actions bot locked and limited conversation to collaborators Jan 24, 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.

None yet

3 participants