System.Resources.ResourceManager is not thread safe when ignore case is enabled #74052

longnlp · 2022-08-17T03:15:29Z

Description

I'm using ResrouceManager to access the resources file in satellite assemblies, and the function is working fine as expected, however I cannot get expected value if I turn on the ignore case for the Resrouce Manager.

Reproduction Steps

I prepared a resource file which has 26 strings, the name is capital letter, and the value is lowercase letter. for example: A -> a; B -> b; ...; Y -> y; Z -> z
I created 52 tasks to call the GetString method to get value for all alphabet ( including capital letters and lower case letters)

Following is the validation code to verify the value

   static void TestResource(ResourceManager manager, string key, Flags flags)
    {
        try
        {
            var value = manager.GetString(key);

            if(!key.ToLower().Equals(value))
            {
                flags.Exception = true;
                Console.WriteLine($"{key} -> {value}");
            }
        }
        catch (Exception ex)
        {
            Console.WriteLine(ex.ToString());
        }

    }

Expected behavior

The expected value should be the lower case of the key.

Actual behavior

Below is the output, you can see the the value for E is not e, it's ☺, and some tasks have exception to get value.

E -> ☺
System.FormatException: Corrupt .resources file. Invalid offset '23593217' into data section.
at System.Resources.ResourceReader.AllocateStringForNameIndex(Int32 index, Int32& dataOffset)
at System.Resources.ResourceReader.ResourceEnumerator.get_Key()
at System.Resources.RuntimeResourceSet.GetObject(String key, Boolean ignoreCase, Boolean isString)
at System.Resources.ResourceManager.GetString(String name, CultureInfo culture)
at System.Resources.ResourceManager.GetString(String name)
at MyApp.Program.TestResource(ResourceManager manager, String key, Flags flags)

Regression?

No response

Known Workarounds

The workaround is to add lock for the GetString method to prevent concurrent access.

Configuration

Which version of .NET is the code running on?
.Net 6
What OS and version, and what distro if applicable?
Windows 11, [Version 10.0.22000.856]
What is the architecture (x64, x86, ARM, ARM64)?
x64
Do you know whether it is specific to that configuration?
I don't think so.
If you're using Blazor, which web browser(s) do you see this issue in?
N/A

Other information

The GetObject method in RuntimeResourceSet has two lock, and I believe we should use one lock for all cases because the second lock cannot lock it when multiple threads call this method.

The text was updated successfully, but these errors were encountered:

dotnet-issue-labeler · 2022-08-17T03:15:33Z

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 · 2022-08-17T04:41:56Z

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

Issue Details

Description

I'm using ResrouceManager to access the resources file in satellite assemblies, and the function is working fine as expected, however I cannot get expected value if I turn on the ignore case for the Resrouce Manager.

Reproduction Steps

I prepared a resource file which has 26 strings, the name is capital letter, and the value is lowercase letter. for example: A -> a; B -> b; ...; Y -> y; Z -> z
I created 52 tasks to call the GetString method to get value for all alphabet ( including capital letters and lower case letters)

Following is the validation code to verify the value

   static void TestResource(ResourceManager manager, string key, Flags flags)
    {
        try
        {
            var value = manager.GetString(key);

            if(!key.ToLower().Equals(value))
            {
                flags.Exception = true;
                Console.WriteLine($"{key} -> {value}");
            }
        }
        catch (Exception ex)
        {
            Console.WriteLine(ex.ToString());
        }

    }

Expected behavior

The expected value should be the lower case of the key.

Actual behavior

Below is the output, you can see the the value for E is not e, it's ☺, and some tasks have exception to get value.

E -> ☺
System.FormatException: Corrupt .resources file. Invalid offset '23593217' into data section.
at System.Resources.ResourceReader.AllocateStringForNameIndex(Int32 index, Int32& dataOffset)
at System.Resources.ResourceReader.ResourceEnumerator.get_Key()
at System.Resources.RuntimeResourceSet.GetObject(String key, Boolean ignoreCase, Boolean isString)
at System.Resources.ResourceManager.GetString(String name, CultureInfo culture)
at System.Resources.ResourceManager.GetString(String name)
at MyApp.Program.TestResource(ResourceManager manager, String key, Flags flags)

Regression?

No response

Known Workarounds

The workaround is to add lock for the GetString method to prevent concurrent access.

Configuration

Which version of .NET is the code running on?
.Net 6
What OS and version, and what distro if applicable?
Windows 11, [Version 10.0.22000.856]
What is the architecture (x64, x86, ARM, ARM64)?
x64
Do you know whether it is specific to that configuration?
I don't think so.
If you're using Blazor, which web browser(s) do you see this issue in?
N/A

Other information

The GetObject method in RuntimeResourceSet has two lock, and I believe we should use one lock for all cases because the second lock cannot lock it when multiple threads call this method.

Author:	longnlp
Assignees:	-
Labels:	`area-System.Resources`, `untriaged`
Milestone:	-

danmoseley · 2022-08-17T04:44:32Z

That seems like a bug as GetString should be thread safe.
It would be interesting to know whether this bug exists for .NET Framework.

tarekgh · 2022-08-17T16:31:14Z

@danmoseley looks you are correct. Some fields in the ResourceReader should be accessed inside a lock but this does not happen in all cases. _store is a good example of that.

danmoseley · 2022-08-17T17:16:31Z

The locking in this and its related classes needs some auditing, and it would probably be helpful to add some Debug.Assert(Monitor.IsEntered(..)) and comments to make clear where protection is expected, what is shared state etc so it can be reasoned about more easily.

buyaa-n · 2022-08-17T23:12:54Z

Doesn't seem regression, adding to future milestone for now, @danmoseley @tarekgh feel free to prioritize as needed

danmoseley · 2022-08-18T00:04:43Z

should we mark help wanted?

madelson · 2022-08-30T22:42:51Z

I'd be interested in working on this.

danmoseley · 2022-08-31T01:57:29Z

It's yours @madelson

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

* 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

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

…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]>

…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]>

ghost added the untriaged New issue has not been triaged by the area owner label Aug 17, 2022

danmoseley added the area-System.Resources label Aug 17, 2022

buyaa-n added this to the Future milestone Aug 17, 2022

ghost removed the untriaged New issue has not been triaged by the area owner label Aug 17, 2022

buyaa-n added the help wanted [up-for-grabs] Good issue for external contributors label Aug 18, 2022

buyaa-n assigned madelson Aug 30, 2022

buyaa-n removed the help wanted [up-for-grabs] Good issue for external contributors label Aug 30, 2022

madelson mentioned this issue Aug 31, 2022

Deadlock when enumerating resources with ResourceManager #74868

Closed

madelson mentioned this issue Sep 3, 2022

Fix thread-safety issues with enumerating ResourceManager. #75054

Merged

ghost added the in-pr There is an active PR which will close this issue when it is merged label Sep 3, 2022

stephentoub closed this as completed in #75054 Jan 3, 2023

ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 3, 2023

tarekgh mentioned this issue Jan 18, 2023

ResourceManager not threadsafe when case insensitive keys are used #80790

Open

jeffhandley mentioned this issue Jan 30, 2023

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

Merged

2 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

System.Resources.ResourceManager is not thread safe when ignore case is enabled #74052

System.Resources.ResourceManager is not thread safe when ignore case is enabled #74052

longnlp commented Aug 17, 2022

dotnet-issue-labeler bot commented Aug 17, 2022

ghost commented Aug 17, 2022

Description

Reproduction Steps

Expected behavior

Actual behavior

Regression?

Known Workarounds

Configuration

Other information

danmoseley commented Aug 17, 2022

tarekgh commented Aug 17, 2022

danmoseley commented Aug 17, 2022

buyaa-n commented Aug 17, 2022

danmoseley commented Aug 18, 2022

madelson commented Aug 30, 2022

danmoseley commented Aug 31, 2022

System.Resources.ResourceManager is not thread safe when ignore case is enabled #74052

System.Resources.ResourceManager is not thread safe when ignore case is enabled #74052

Comments

longnlp commented Aug 17, 2022

Description

Reproduction Steps

Expected behavior

Actual behavior

Regression?

Known Workarounds

Configuration

Other information

dotnet-issue-labeler bot commented Aug 17, 2022

ghost commented Aug 17, 2022

Description

Reproduction Steps

Expected behavior

Actual behavior

Regression?

Known Workarounds

Configuration

Other information

danmoseley commented Aug 17, 2022

tarekgh commented Aug 17, 2022

danmoseley commented Aug 17, 2022

buyaa-n commented Aug 17, 2022

danmoseley commented Aug 18, 2022

madelson commented Aug 30, 2022

danmoseley commented Aug 31, 2022