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

TestResourceManagerIsSafeForConcurrentAccessAndEnumeration fails intermittently in CI #80277

Closed
jkotas opened this issue Jan 6, 2023 · 4 comments · Fixed by #80330
Closed
Labels
area-System.Resources blocking-clean-ci Blocking PR or rolling runs of 'runtime' or 'runtime-extra-platforms' Known Build Error Use this to report build issues in the .NET Helix tab

Comments

@jkotas
Copy link
Member

jkotas commented Jan 6, 2023

Build Information

Build: https://dev.azure.com/dnceng-public/cbb18261-c48f-4abb-8651-8cdcb5474649/_build/results?buildId=124218
Build error leg or test failing: System.Resources.Extensions.Tests.PreserializedResourceWriterTests.TestResourceManagerIsSafeForConcurrentAccessAndEnumeration
Pull request: #79970

Error Message

Fill the error message using known issues guidance.

{
  "ErrorMessage": "TestResourceManagerIsSafeForConcurrentAccessAndEnumeration",
  "BuildRetry": false
}

Report

Build Definition Step Name Console log Pull Request
129536 dotnet/runtime Send to Helix Log #66551
129348 dotnet/runtime Send to Helix Log
128306 dotnet/runtime Send to Helix Log #80182
127864 dotnet/runtime Send to Helix Log
127226 dotnet/runtime Send to Helix Log #79169
126565 dotnet/runtime Send to Helix Log #74428
126238 dotnet/runtime Send to Helix Log #79771
126132 dotnet/runtime Send to Helix Log
Build Definition Test Pull Request
129536 dotnet/runtime System.Resources.Extensions.Tests.PreserializedResourceWriterTests.TestResourceManagerIsSafeForConcurrentAccessAndEnumeration #66551
129348 dotnet/runtime System.Resources.Extensions.Tests.PreserializedResourceWriterTests.TestResourceManagerIsSafeForConcurrentAccessAndEnumeration
128306 dotnet/runtime System.Resources.Extensions.Tests.PreserializedResourceWriterTests.TestResourceManagerIsSafeForConcurrentAccessAndEnumeration #80182
127864 dotnet/runtime System.Resources.Extensions.Tests.PreserializedResourceWriterTests.TestResourceManagerIsSafeForConcurrentAccessAndEnumeration
127226 dotnet/runtime System.Resources.Extensions.Tests.PreserializedResourceWriterTests.TestResourceManagerIsSafeForConcurrentAccessAndEnumeration #79169
126565 dotnet/runtime System.Resources.Extensions.Tests.PreserializedResourceWriterTests.TestResourceManagerIsSafeForConcurrentAccessAndEnumeration #74428
126238 dotnet/runtime System.Resources.Extensions.Tests.PreserializedResourceWriterTests.TestResourceManagerIsSafeForConcurrentAccessAndEnumeration #79771
126132 dotnet/runtime System.Resources.Extensions.Tests.PreserializedResourceWriterTests.TestResourceManagerIsSafeForConcurrentAccessAndEnumeration

Summary

24-Hour Hit Count 7-Day Hit Count 1-Month Count
4 16 16
@jkotas jkotas added blocking-clean-ci Blocking PR or rolling runs of 'runtime' or 'runtime-extra-platforms' Known Build Error Use this to report build issues in the .NET Helix tab labels Jan 6, 2023
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 6, 2023
@ghost
Copy link

ghost commented Jan 6, 2023

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

Issue Details

Build Information

Build: https://dev.azure.com/dnceng-public/cbb18261-c48f-4abb-8651-8cdcb5474649/_build/results?buildId=124218
Build error leg or test failing: System.Resources.Extensions.Tests.PreserializedResourceWriterTests.TestResourceManagerIsSafeForConcurrentAccessAndEnumeration
Pull request: #79970

Error Message

Fill the error message using known issues guidance.

{
  "ErrorMessage": "TestResourceManagerIsSafeForConcurrentAccessAndEnumeration",
  "BuildRetry": false
}
Author: jkotas
Assignees: -
Labels:

area-System.Resources, blocking-clean-ci, untriaged, Known Build Error

Milestone: -

@jkotas
Copy link
Member Author

jkotas commented Jan 6, 2023

This is a new test added in #75054.

cc @madelson @joperezr

@madelson
Copy link
Contributor

madelson commented Jan 7, 2023

It looks like this is only for the version of this test in System.Resources.Extensions and that the failure is a timeout.

I recall getting feedback to raise the timeout on the original PR, but it looks like I forgot to do it for this version of the test :-/.

Filed a PR to make the two timeouts the same; hopefully this is sufficient to address.

@ghost ghost removed in-pr There is an active PR which will close this issue when it is merged untriaged New issue has not been triaged by the area owner labels Jan 7, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 7, 2023
carlossanlop pushed a commit that referenced this issue Mar 10, 2023
…er. (#81283)

* Fix thread-safety issues with enumerating ResourceManager.

Concurrently enumerating a ResourceManager while also calling GetString()
and similar methods was prone to both transient errors and deadlock.

The transient errors were caused by RuntimeResourceSet calling internal
methods on ResourceReader that did not properly lock. Now, all exposed
methods on the Reader are thread-safe.

The deadlock was called by inconsistent lock ordering between
ResourceReader.ResourceEnumerator and RuntimeResourceSet which both lock
on the RuntimeResourceSet's cache and on the ResourceReader itself. Now,
the enumerator does not need to take both locks at the same time.

Fix #74052
Fix #74868

* Remove trailing whitespace

* Address feedback from #75054

* Add comment in response to #75054 (comment)

* Implement feedback from #75054

* Increase timeout for TestResourceManagerIsSafeForConcurrentAccessAndEnumeration (#80330)

This raises the timeout to 30s, the same as what we have for the equivalent ResourceManager test (https://github.com/dotnet/runtime/blob/15fcb990fe17348ab6ddde0939200b900169920b/src/libraries/System.Resources.ResourceManager/tests/ResourceManagerTests.cs#L255).

fix #80277

---------

Co-authored-by: Michael Adelson <[email protected]>
Co-authored-by: madelson <[email protected]>
Co-authored-by: Buyaa Namnan <[email protected]>
carlossanlop added a commit that referenced this issue Mar 10, 2023
…er. (#81281)

* Fix thread-safety issues with enumerating ResourceManager.

Concurrently enumerating a ResourceManager while also calling GetString()
and similar methods was prone to both transient errors and deadlock.

The transient errors were caused by RuntimeResourceSet calling internal
methods on ResourceReader that did not properly lock. Now, all exposed
methods on the Reader are thread-safe.

The deadlock was called by inconsistent lock ordering between
ResourceReader.ResourceEnumerator and RuntimeResourceSet which both lock
on the RuntimeResourceSet's cache and on the ResourceReader itself. Now,
the enumerator does not need to take both locks at the same time.

Fix #74052
Fix #74868

* Remove trailing whitespace

* Address feedback from #75054

* Add comment in response to #75054 (comment)

* Implement feedback from #75054

* Increase timeout for TestResourceManagerIsSafeForConcurrentAccessAndEnumeration (#80330)

This raises the timeout to 30s, the same as what we have for the equivalent ResourceManager test (https://github.com/dotnet/runtime/blob/15fcb990fe17348ab6ddde0939200b900169920b/src/libraries/System.Resources.ResourceManager/tests/ResourceManagerTests.cs#L255).

fix #80277

---------

Co-authored-by: Michael Adelson <[email protected]>
Co-authored-by: madelson <[email protected]>
Co-authored-by: Carlos Sanchez <[email protected]>
Co-authored-by: Buyaa Namnan <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Resources blocking-clean-ci Blocking PR or rolling runs of 'runtime' or 'runtime-extra-platforms' Known Build Error Use this to report build issues in the .NET Helix tab
Projects
None yet
2 participants