Fix thread-safety issues with enumerating ResourceManager. #75054

madelson · 2022-09-03T11:45:04Z

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

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 dotnet#74052 Fix dotnet#74868

ghost · 2022-09-03T11:45:20Z

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

Issue Details

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

Author:	madelson
Assignees:	-
Labels:	`area-System.Resources`
Milestone:	-

madelson · 2022-09-03T11:46:04Z

src/libraries/System.Private.CoreLib/src/System/Resources/ResourceReader.Core.cs

            "the user to only get one error.")]
        private object DeserializeObject(int typeIndex)
        {
+            Debug.Assert(Monitor.IsEntered(this));


I added these to private methods that access _store per this suggestion.

src/libraries/System.Private.CoreLib/src/System/Resources/ResourceReader.cs

madelson · 2022-09-03T11:52:11Z

src/libraries/System.Private.CoreLib/src/System/Resources/RuntimeResourceSet.cs


-            // Normally calling LoadString or LoadObject requires
-            // taking a lock.  Note that in this case, we took a
-            // lock before calling this method.


This comment was not (no longer?) correct. We do typically call these within a lock, but it is a lock on either _resCache or _caseInsensitiveTable. That lock was previously making things thread-safe so long as the caller was only using GetString/GetObject and was not flipping ResourceManager.IgnoreCase on and off.

Now, LoadString/LoadObject do lock internally so there is no edge case here.

madelson · 2022-09-03T11:59:46Z

src/libraries/System.Resources.ResourceManager/tests/ResourceManagerTests.cs

+            using Barrier barrier = new(Threads);
+            Task task = Task.WhenAll(Enumerable.Range(0, Threads).Select(_ => Task.Run(WaitForBarrierThenEnumerateResources)));
+
+            Assert.True(task.Wait(TimeSpan.FromSeconds(10)));


Before my fixes, this test reliably would reproduce both issues on my machine. Let me know if there is a better way to test for concurrency issues like this.

I do think it is very important to have such a test, because this problem was actually previously fixed in 2015 and then broken again in 2020 (I think the break happened here).

madelson · 2022-09-03T12:00:39Z

...braries/System.Resources.ResourceManager/tests/System.Resources.ResourceManager.Tests.csproj

-    <Content Include="Resources\icon.ico"
-          Link="%(Filename)%(Extension)"
-          CopyToOutputDirectory="PreserveNewest"
-          Visible="false" />


Reformatting was done by VS

madelson · 2022-09-03T12:01:42Z

...braries/System.Resources.ResourceManager/tests/System.Resources.ResourceManager.Tests.csproj

+    <EmbeddedResource Include="Resources\AToZResx.resx">
+      <Generator>ResXFileCodeGenerator</Generator>
+      <LastGenOutput>AToZResx.Designer.cs</LastGenOutput>
+    </EmbeddedResource>


This is what VS generated when I made a new resx file. Any reason to change it to match what the other files were using?

It'd be nice to be consistent.

nvm reading this again I think it is consistent with the others

src/libraries/System.Private.CoreLib/src/System/Resources/ResourceReader.cs

joperezr · 2022-11-08T21:09:34Z

src/libraries/System.Private.CoreLib/src/System/Resources/ResourceReader.Core.cs

            "the user to only get one error.")]
        private object DeserializeObject(int typeIndex)
        {
+            Debug.Assert(Monitor.IsEntered(this));


Can we also add a comment explaining why this is being added?

After double checking I don't think we need this one; this method doesn't access _store directly. Removed

joperezr · 2022-11-08T21:10:36Z

src/libraries/System.Private.CoreLib/src/System/Resources/ResourceReader.Core.cs

            "Custom readers as well as custom objects on the resources file are not observable by the trimmer and so required assemblies, types and members may be removed.")]
        private bool InitializeBinaryFormatter()
        {
+            Debug.Assert(Monitor.IsEntered(this));


Same here. That way in the future people that look at this code will know why is the protection expected.

Removed this one too. Not needed.

joperezr · 2022-11-08T21:17:23Z

src/libraries/System.Resources.ResourceManager/tests/ResourceManagerTests.cs

+        [Theory]
+        [InlineData(false)]
+        [InlineData(true)]
+        public static void TestResourceManagerIsSafeForConcurrentAccessAndEnumeration(bool useEnumeratorEntry)


Does it make sense to add equivalent tests in System.Resources.Extensions as well and perhaps with a resource file that has something other than strings?

joperezr

Sorry for the delay in reviewing this @madelson, I've left some comments but overall this is looking good, thanks for the contribution.

madelson · 2022-11-09T03:39:14Z

I've left some comments but overall this is looking good

Thanks @joperezr . I've pushed a change addressing your feedback. Let me know if there is anything else to do.

joperezr · 2022-11-15T21:22:33Z

    private void SkipString()

What about private void SkipString()? That one seems to be using _store as well.

joperezr · 2022-11-15T21:26:11Z

Same for private unsafe string AllocateStringForNameIndex(int index, out int dataOffset) that one also seems to be accessing _store

joperezr · 2022-11-15T21:30:07Z

So following the lines of my last two comments, there seems to be two different approaches when _store gets accessed outside of the constructor:

You use lock(this)
You don't need to use lock(this) given the method could only be called by something that is already locked, so you add a comment to mention that on the method.

There are some places where we still use _store and are not doing either of the above (my assumption is the fall into the second category), so I t hink it would be good to add a comment to those places to explain why is locking not required, as the comment you added on _store field itself requires this.

madelson · 2022-11-15T23:48:55Z

What about private void SkipString()? That one seems to be using _store as well.

@joperezr this one can't have an assertion because it is called both during initialization (which per my comment on _store does not use locking) and post-initialization.

Do you think it is worth adding a comment like this to SkipString()?:

// Note: this method assumes that it is called either from the constructor or within another method that locks on this

Same for private unsafe string AllocateStringForNameIndex(int index, out int dataOffset) that one also seems to be accessing _store

This one accesses _store but note that the access is wrapped in a lock(this) block within that method. I only added assertions to methods that make assumptions about locking happening in the caller.

There are some places where we still use _store and are not doing either of the above

I believe all of those places are called during initialization, which is called out by the comment on _store:

        // Backing store we're reading from. Usages outside of constructor
        // initialization must be protected by lock (this).

In cases which are always called during initialization (e.g. _ReadResources()) I assert that the lock is not taken.

joperezr · 2022-11-16T18:23:03Z

I see thanks for the explanation and sorry that I missed some of those comments.

Do you think it is worth adding a comment like this to SkipString()?:

Yes, I do think this would be good for consistency.

joperezr

Apart from my last NIT of just adding a comment to SkipString this change looks good to me. @stephentoub since you commented on this PR earlier, any thoughts on the reply from @madelson?

stephentoub · 2022-12-09T15:02:53Z

src/libraries/System.Resources.ResourceManager/tests/ResourceManagerTests.cs

+
+            const int Threads = 10;
+            using Barrier barrier = new(Threads);
+            Task task = Task.WhenAll(Enumerable.Range(0, Threads).Select(_ => Task.Run(WaitForBarrierThenEnumerateResources)));


All of the same comments as above.

stephentoub

A few comments, but otherwise LGTM. Thanks.

madelson · 2022-12-12T00:35:26Z

@joperezr looking at the new build failures these seem unrelated? https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-75054-merge-b07193f9df6f4ac4a6/JIT.1/1/console.735a06a0.log?helixlogtype=result what do you think?

jkotas · 2022-12-12T02:11:37Z

The new failure is #79517

madelson · 2022-12-16T12:28:45Z

@joperezr anything else to do here?

buyaa-n · 2023-01-27T18:12:27Z

/backport to release/6.0

github-actions · 2023-01-27T18:12:39Z

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/4027066289

joperezr · 2023-01-27T18:24:37Z

@buyaa-n assuming you also plan to backport to 7.0?

buyaa-n · 2023-01-27T18:30:01Z

/backport to release/7.0

github-actions · 2023-01-27T18:30:14Z

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/4027191479

ghost added area-System.Resources community-contribution Indicates that the PR has been added by a community member labels Sep 3, 2022

madelson commented Sep 3, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Resources/ResourceReader.cs Show resolved Hide resolved

madelson commented Sep 3, 2022

View reviewed changes

Remove trailing whitespace

0c274c1

stephentoub reviewed Sep 6, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Resources/ResourceReader.cs Show resolved Hide resolved

madelson requested a review from stephentoub September 6, 2022 23:40

joperezr requested a review from ericstj September 29, 2022 22:58

ericstj removed their request for review November 7, 2022 20:11

joperezr reviewed Nov 8, 2022

View reviewed changes

Address feedback from dotnet#75054

5d99bf6

build-analysis bot mentioned this pull request Nov 9, 2022

Tracking issue for CI build timeouts #76454

Closed

madelson requested review from joperezr and removed request for stephentoub November 15, 2022 23:49

joperezr approved these changes Nov 16, 2022

View reviewed changes

stephentoub reviewed Dec 9, 2022

View reviewed changes

stephentoub approved these changes Dec 9, 2022

View reviewed changes

madelson added 2 commits December 9, 2022 21:40

Merge remote-tracking branch 'origin/main' into fix-74052-and-74868

a6278d0

Implement feedback from dotnet#75054

eea9c08

build-analysis bot mentioned this pull request Dec 12, 2022

Test failure JIT\\opt\\Tailcall\\TailcallVerifyWithPrefix\\TailcallVerifyWithPrefix.cmd #79517

Closed

Merge branch 'dotnet:main' into fix-74052-and-74868

7307995

build-analysis bot mentioned this pull request Dec 13, 2022

[wasm] Library tests failing during linking for AOT - SIGKILL #79569

Closed

stephentoub approved these changes Jan 3, 2023

View reviewed changes

stephentoub merged commit 15fcb99 into dotnet:main Jan 3, 2023

jkotas mentioned this pull request Jan 6, 2023

TestResourceManagerIsSafeForConcurrentAccessAndEnumeration fails intermittently in CI #80277

Closed

github-actions bot mentioned this pull request Jan 27, 2023

[release/6.0] Fix thread-safety issues with enumerating ResourceManager. #81281

Merged

2 tasks

github-actions bot pushed a commit that referenced this pull request Jan 27, 2023

Address feedback from #75054

e063428

github-actions bot pushed a commit that referenced this pull request Jan 27, 2023

Add comment in response to #75054 (comment)

d42bd87

github-actions bot pushed a commit that referenced this pull request Jan 27, 2023

Implement feedback from #75054

c111a0e

github-actions bot mentioned this pull request Jan 27, 2023

[release/7.0] Fix thread-safety issues with enumerating ResourceManager. #81283

Merged

2 tasks

github-actions bot pushed a commit that referenced this pull request Jan 27, 2023

Address feedback from #75054

99a8c22

github-actions bot pushed a commit that referenced this pull request Jan 27, 2023

Add comment in response to #75054 (comment)

8e93dca

github-actions bot pushed a commit that referenced this pull request Jan 27, 2023

Implement feedback from #75054

e88ea8d

carlossanlop pushed a commit that referenced this pull request Feb 10, 2023

Address feedback from #75054

f215ba2

carlossanlop pushed a commit that referenced this pull request Feb 10, 2023

Add comment in response to #75054 (comment)

478b697

carlossanlop pushed a commit that referenced this pull request Feb 10, 2023

Implement feedback from #75054

faba965

ghost locked as resolved and limited conversation to collaborators Feb 26, 2023

Fix thread-safety issues with enumerating ResourceManager. #75054

Fix thread-safety issues with enumerating ResourceManager. #75054

Uh oh!

Conversation

madelson commented Sep 3, 2022

Uh oh!

ghost commented Sep 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

madelson Sep 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joperezr left a comment

Choose a reason for hiding this comment

Uh oh!

madelson commented Nov 9, 2022

Uh oh!

joperezr commented Nov 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joperezr commented Nov 15, 2022

Uh oh!

joperezr commented Nov 15, 2022

Uh oh!

madelson commented Nov 15, 2022

Uh oh!

joperezr commented Nov 16, 2022

Uh oh!

joperezr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

madelson commented Dec 12, 2022

Uh oh!

jkotas commented Dec 12, 2022

Uh oh!

madelson commented Dec 16, 2022

Uh oh!

buyaa-n commented Jan 27, 2023

Uh oh!

github-actions bot commented Jan 27, 2023

Uh oh!

joperezr commented Jan 27, 2023

Uh oh!

buyaa-n commented Jan 27, 2023

Uh oh!

github-actions bot commented Jan 27, 2023

Uh oh!

Reviewers

Assignees

madelson Sep 3, 2022 •

edited

Loading

joperezr commented Nov 15, 2022 •

edited

Loading