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

Update dependencies from dotnet/roslyn #110105

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

am11
Copy link
Member

@am11 am11 commented Nov 23, 2024

Testing #110084 with build fixes.

dotnet-maestro bot and others added 3 commits November 22, 2024 13:09
…122.2

Microsoft.SourceBuild.Intermediate.roslyn , Microsoft.CodeAnalysis , Microsoft.CodeAnalysis.CSharp , Microsoft.Net.Compilers.Toolset
 From Version 4.13.0-2.24570.4 -> To Version 4.13.0-2.24572.2
…122.15

Microsoft.SourceBuild.Intermediate.roslyn , Microsoft.CodeAnalysis , Microsoft.CodeAnalysis.CSharp , Microsoft.Net.Compilers.Toolset
 From Version 4.13.0-2.24570.4 -> To Version 4.13.0-2.24572.15
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Nov 23, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

current = current._next;
yield return value;
Copy link
Member Author

Choose a reason for hiding this comment

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

@jjonescz, without this change, the tests were consistently failing:

$ ./build.sh libs+libs.tests
$ ./dotnet.sh build -t:Test src/libraries/System.Collections.Concurrent/tests
...
    Discovering: System.Collections.Concurrent.Tests (method display = ClassAndMethod, method display options = None)
    Discovered:  System.Collections.Concurrent.Tests (found 901 of 909 test cases)
    Starting:    System.Collections.Concurrent.Tests (parallel test collections = on [10 threads], stop on fail = off)
      System.Collections.Concurrent.Tests.ConcurrentStackTests.Enumerator_MoveNext_AfterDisposal(count: 1) [FAIL]
        System.NullReferenceException : Object reference not set to an instance of an object.
        Stack Trace:
          /Users/am11/projects/runtime5/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentStack.cs(743,0): at System.Collections.Concurrent.ConcurrentStack`1.GetEnumerator(Node head)+MoveNext()
          /Users/am11/projects/runtime5/src/libraries/Common/tests/System/Collections/IEnumerable.Generic.Tests.cs(395,0): at System.Collections.Tests.IEnumerable_Generic_Tests`1.Enumerator_MoveNext_AfterDisposal(Int32 count)
             at InvokeStub_IEnumerable_Generic_Tests`1.Enumerator_MoveNext_AfterDisposal(Object, Span`1)
          /Users/am11/projects/runtime5/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBaseInvoker.cs(95,0): at System.Reflection.MethodBaseInvoker.InvokeWithOneArg(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
      System.Collections.Concurrent.Tests.ConcurrentStackTests.Enumerator_MoveNext_AfterDisposal(count: 75) [FAIL]
        System.NullReferenceException : Object reference not set to an instance of an object.
        Stack Trace:
          /Users/am11/projects/runtime5/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentStack.cs(743,0): at System.Collections.Concurrent.ConcurrentStack`1.GetEnumerator(Node head)+MoveNext()
          /Users/am11/projects/runtime5/src/libraries/Common/tests/System/Collections/IEnumerable.Generic.Tests.cs(395,0): at System.Collections.Tests.IEnumerable_Generic_Tests`1.Enumerator_MoveNext_AfterDisposal(Int32 count)
             at InvokeStub_IEnumerable_Generic_Tests`1.Enumerator_MoveNext_AfterDisposal(Object, Span`1)
          /Users/am11/projects/runtime5/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBaseInvoker.cs(95,0): at System.Reflection.MethodBaseInvoker.InvokeWithOneArg(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
    Finished:    System.Collections.Concurrent.Tests
  === TEST EXECUTION SUMMARY ===
     System.Collections.Concurrent.Tests  Total: 2460, Errors: 0, Failed: 2, Skipped: 0, Time: 74,172s
...

is it expected?

Copy link
Member

Choose a reason for hiding this comment

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

@jcouv is this due to dotnet/roslyn#75908?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that is the cause.

Here's a sharplab repro.

In the "main" branch, we NRE.
In the "default" branch (ie. before the change to clear locals during disposal), we printed a result without crash. But it looks like that result was incorrect already...
We were incorrectly allowing enumeration to continue.

The correct behavior would be for disposal to set the enumerator state to "after" (thereby causing MoveNext to return false thereafter).
This issue becomes more observable after the change to clear local state (NRE in the present scenario).
Filed dotnet/roslyn#76078

I will discuss with @AlekseyTs @333fred on how best to proceed. I thinking to revert the change temporarily and fix the disposal bug first.

Copy link
Member

Choose a reason for hiding this comment

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

@am11 There's a couple of different ways we can proceed. Let me propose one:
I'm thinking to keep the roslyn change and fix the unearthed bug asap. Due to Thanksgiving, this means I could probably have an updated build of roslyn by next Wednesday (Dec 4th).
So you would either have to put this PR on hold until then, or you'd have to temporarily skip the failing tests (that do Dispose then MoveNext on compiler-generated enumerators) to merge this PR.
Does that sound okay?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jcouv, thanks for the info. I think we can wait, since there are some other NRE failures in CI which appear to be related.

Copy link
Member

Choose a reason for hiding this comment

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

That works, thanks.
FWIW, here's the fix in progress

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Meta community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants