BREAKING: Fix `InvalidOperationException` when using PropertiesDictionary in a multithreaded application and remove [Serializable] for it. #2415

shaopeng-gh · 2021-12-09T09:15:54Z

Description:

Bug: InvalidOperationException when using PropertiesDictionary in a multithreaded application.
It is caused by use of Dictionary type instead of ConcurrentDictionary type.

Fixes:

Change TypedPropertiesDictionary to use ConcurrentDictionary.
also found a existing issue with method SavePropertiesToXmlStream, fix by the way.

BREAKING: removed [Serializable] for TypedPropertiesDictionary.
[Serializable] is not for json and xml serialization, it is only for BinaryFormatter serialization.
For SDK user that is currently using BinaryFormatter on TypedPropertiesDictionary object,
will result in error:
SerializationException: Type PropertiesDictionary is not marked as serializable

Error Details:

InvalidOperationException: Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.
at System.Collections.Generic.Dictionary2.FindEntry(TKey key) at System.Collections.Generic.Dictionary2.TryGetValue(TKey key, TValue& value)
at Microsoft.CodeAnalysis.Sarif.PropertiesDictionary.TryGetProperty[T](String key, T& value)
at Microsoft.CodeAnalysis.Sarif.PropertiesDictionary.GetProperty[T](PerLanguageOption1 setting, Boolean cacheDefault) at Microsoft.CodeAnalysis.Sarif.PropertiesDictionary.GetProperty[T](PerLanguageOption1 setting)
at Microsoft.CodeAnalysis.IL.Rules.EnableSpectreMitigations.AnalyzePortableExecutableAndPdb(BinaryAnalyzerContext context) in D:\a\r1\a\src\BinSkim.Rules\PERules\BA2024.EnableSpectreMitigations.cs:line 139
at Microsoft.CodeAnalysis.IL.Rules.WindowsBinaryAndPdbSkimmerBase.Analyze(BinaryAnalyzerContext context) in D:\a\r1\a\src\BinSkim.Rules\PERules\WindowsBinaryAndPdbSkimmerBase.cs:line 63
at Microsoft.CodeAnalysis.Sarif.Driver.MultithreadedAnalyzeCommandBase2.AnalyzeTargetHelper(TContext context, IEnumerable1 skimmers, ISet`1 disabledSkimmers)

Test Case result before Fix:

Microsoft.CodeAnalysis.Sarif.PropertiesDictionaryTests.PropertiesDictionary_ConcurrentReadAndWrite
Duration: 389 ms

Message:
Assert.Null() Failure
Expected: (null)
Actual: System.AggregateException: One or more errors occurred. (Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.) (Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.)
---> System.InvalidOperationException: Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.
at System.Collections.Generic.Dictionary2.FindEntry(TKey key) at System.Collections.Generic.Dictionary2.TryGetValue(TKey key, TValue& value)
at Microsoft.CodeAnalysis.Sarif.PropertiesDictionary.GetSettingsContainer(IOption setting, Boolean cacheDefault) in C:\GH\s-s\src\Sarif\PropertiesDictionary.cs:line 126
at Microsoft.CodeAnalysis.Sarif.PropertiesDictionary.SetProperty(IOption setting, Object value, Boolean cacheDescription, Boolean persistToSettingsContainer) in C:\GH\s-s\src\Sarif\PropertiesDictionary.cs:line 78
at Microsoft.CodeAnalysis.Sarif.PropertiesDictionary.SetProperty(IOption setting, Object value, Boolean cacheDescription) in C:\GH\s-s\src\Sarif\PropertiesDictionary.cs:line 70
at Microsoft.CodeAnalysis.Sarif.PropertiesDictionary.SetProperty(IOption setting, Object value) in C:\GH\s-s\src\Sarif\PropertiesDictionary.cs:line 65
at Microsoft.CodeAnalysis.Sarif.PropertiesDictionaryTests.<>c__DisplayClass16_1.<PropertiesDictionary_ConcurrentReadAndWrite>b__1() in C:\GH\s-s\src\Test.UnitTests.Sarif\PropertiesDictionaryTests.cs:line 146
at System.Threading.Tasks.Task.InnerInvoke()
at System.Threading.Tasks.Task.<>c.<.cctor>b__274_0(Object obj)
at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread threadPoolThread, ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location where exception was thrown ---
at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread threadPoolThread, ExecutionContext executionContext, ContextCallback callback, Object state)
at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot, Thread threadPoolThread)
--- End of inner exception stack trace ---
at System.Threading.Tasks.Task.WaitAllCore(Task[] tasks, Int32 millisecondsTimeout, CancellationToken cancellationToken)
at System.Threading.Tasks.Task.WaitAll(Task[] tasks)
at Microsoft.CodeAnalysis.Sarif.PropertiesDictionaryTests.<>c__DisplayClass16_0.<PropertiesDictionary_ConcurrentReadAndWrite>b__0() in C:\GH\s-s\src\Test.UnitTests.Sarif\PropertiesDictionaryTests.cs:line 161
at Xunit.Record.Exception(Action testCode) in C:\Dev\xunit\xunit\src\xunit.core\Record.cs:line 26

eddynaka · 2021-12-09T11:05:44Z

src/Sarif/PropertiesDictionary.cs

-        protected PropertiesDictionary(SerializationInfo info, StreamingContext context)
-            : base(info, context)
-        {
-        }


check if the serialization will be the same comparing previous vs current #Closed

got it, thanks

the file written are the same

eddynaka · 2021-12-09T11:06:41Z

            SettingNameToDescriptionsMap = SettingNameToDescriptionsMap ?? new Dictionary<string, string>();

I think you might need to change this as well.
Be sure to test the cacheDescription part

In reply to: 989748904

Refers to: src/Sarif/TypedPropertiesDictionary.cs:72 in bef3fbe. [](commit_id = bef3fbe, deletion_comment = False)

eddynaka · 2021-12-09T11:07:33Z

src/Test.UnitTests.Sarif/PropertiesDictionaryTests.cs

+            Assert.Null(exception);
+        }
+
+


remove one line #Resolved

eddynaka · 2021-12-09T11:08:04Z

src/Test.UnitTests.Sarif/PropertiesDictionaryTests.cs

@@ -192,6 +227,10 @@ private PropertiesDictionary RoundTripThroughJson(PropertiesDictionary propertie
            new PerLanguageOption<StringSet>(
                FEATURE, nameof(StringSetProperty), defaultValue: () => { return STRINGSET_DEFAULT; });

+        public static PerLanguageOption<StringSet> GetStringSetProperty(int i) =>


GetStringSetProperty

I would change to StringGetProperty #Resolved

StringSet is a actually class here, let me know
how about change to "GenerateStringSetProperty"
the Get word may be confusing

'StringSet' is correct. :) I can completely understand why 'GenerateStringGetProperty' seems right, though! :)

shaopeng-gh · 2021-12-09T22:56:56Z

            SettingNameToDescriptionsMap = SettingNameToDescriptionsMap ?? new Dictionary<string, string>();

got it

In reply to: 989748904

Refers to: src/Sarif/TypedPropertiesDictionary.cs:72 in bef3fbe. [](commit_id = bef3fbe, deletion_comment = False)

shaopeng-gh · 2021-12-10T08:33:10Z

            SettingNameToDescriptionsMap = SettingNameToDescriptionsMap ?? new Dictionary<string, string>();

I think the cacheDescription part have problem before our changes. And we don't have a any test for that field.
I have made some changes, testing.

In reply to: 990377643

Refers to: src/Sarif/TypedPropertiesDictionary.cs:72 in bef3fbe. [](commit_id = bef3fbe, deletion_comment = False)

shaopeng-gh · 2021-12-10T10:39:18Z

            SettingNameToDescriptionsMap = SettingNameToDescriptionsMap ?? new Dictionary<string, string>();

added test for the cacheDescription save.

In reply to: 990734565

Refers to: src/Sarif/TypedPropertiesDictionary.cs:72 in bef3fbe. [](commit_id = bef3fbe, deletion_comment = False)

eddynaka · 2021-12-10T17:40:47Z

src/Sarif/PropertiesDictionaryExtensionMethods.cs

@@ -73,33 +74,28 @@ public static class PropertiesDictionaryExtensionMethods
                StringSet stringSet = property as StringSet;
                if (stringSet != null)
                {
+                    SaveSettingNameToDescriptionMap(writer, key, settingNameToDescriptionMap);


SaveSettingNameToDescriptionMap

pls, add tests that cover this change. #Resolved

found this new method is not needed, removed.
the bug was comment not written to file,
the method I add was to test that. PropertiesDictionary_TestCacheDescription

eddynaka · 2021-12-10T17:43:07Z

src/Sarif/TypedPropertiesDictionary.cs

@@ -13,7 +13,7 @@ public interface IMarker { }

    [Serializable]
    [JsonConverter(typeof(TypedPropertiesDictionaryConverter))]
-    public class TypedPropertiesDictionary<T> : Dictionary<string, T>, IMarker where T : new()
+    public class TypedPropertiesDictionary<T> : ConcurrentDictionary<string, T>, IMarker where T : new()


ConcurrentDictionary

Update the title of the PR and add a new entry to the ReleaseHistory.

The title should be something like: "Fix InvalidOperationException when using PropertiesDictionary in a multithreaded application".

With that, if any customer checks the release history, the customer would know what motivated us to change this from Dictionary to ConcurrentDictionary.

This should also be applied to the PR description. #Resolved

done both, thanks

also added the ReleaseHistory for the other 2 PRs.

michaelcfanning · 2021-12-10T20:24:21Z

src/Sarif/PropertiesDictionary.cs

@@ -37,11 +37,6 @@ public PropertiesDictionary() : this(null)
        {
        }

-        protected PropertiesDictionary(SerializationInfo info, StreamingContext context)


PropertiesDictionary

Why did you remove this constructor? Preivously in .NET this ctor was required for all [Serializable] things, has that changed? #Closed

the .net core concurrentdictionary is not binary serializable, so I removed it,

question, do we even need it, it is not for json and xml serialization, it is only for BinaryFormatter serialization, in the solution I don't see code use BinaryFormatter

That's a good question and in future, definitely try to raise this before making this kind of change. Remember we are in an SDK, so it is possible that someone is using BinaryFormatter with this public type (even though we do not as you point out).

I think probably it is unlikely we're breaking someone out there. So this change is fine. However we need to very clearly mark this breaking change in behavior in the release notes.

Imagine someone using BInaryFormatter with this type, using an older SDK. Now they update to the new SDK and everything is broken. Ideally, your release notes would also mention the symptom these users would receive. Make sense? IOW, try to use BinaryFormatter again PropertiesDictionary (once you remove the [Serializable] attribute as well. See what exception is raised. Make sure the release notes reference this exception in case users experience it.

By doing this, we leave an easily followed trail of clues that tell developers what happened. That's the least we can do, given that we're introducing a breaking change. :)

thanks for the great inputs,
I have remove the [Serializable] attribute and ran it, the error is
SerializationException: Type PropertiesDictionary is not marked as serializable
I have added it both in the PR desc and also the release history md file.

@michaelcfanning

additional question, do you want me to create a work item to check and remove all [Serializable] in the solution,
( I have not dig if any of them is used somewhere yet, just to bring up the discussion )

https://docs.microsoft.com/en-us/dotnet/standard/serialization/binaryformatter-security-guide

https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca2300

michaelcfanning · 2021-12-10T20:27:32Z

src/Sarif/ExtensionMethods.cs

@@ -585,5 +586,15 @@ internal static void ConsumeElementOfDepth(this XmlReader xmlReader, int endElem
                xmlReader.Read();
            }
        }
+
+        public static bool Add<T>(this ConcurrentDictionary<string, T> concurrentDictionary, string key, T value)


Add

Why did you add this? What does the CurrentDictionary IDictionary.Add implementation call into? A non-thread-safe base method? #Resolved

just checked the source code of CurrentDictionary,
the IDictionary<TKey, TValue>.Add implementation
did call the same method as the TryAdd
so we can use it thread safely.

Also agree that we should not make the Add has a return value people will get confused.

updated the code.

michaelcfanning · 2021-12-10T20:30:23Z

src/Sarif/ExtensionMethods.cs

@@ -585,5 +586,15 @@ internal static void ConsumeElementOfDepth(this XmlReader xmlReader, int endElem
                xmlReader.Read();
            }
        }
+
+        public static bool Add<T>(this ConcurrentDictionary<string, T> concurrentDictionary, string key, T value)


bool

why does this extension method return a bool? IDictionary.Add returns void.

It's not a good idea to create a strange return value for 'Add', this will confuse experienced .NET programmers.

If you need to override 'Add', so that a concurrent dictionary behaves properly when it flows around to IDictionary-accepting things, this should return void. And in the case where Add would throw, you should consult the return value of TryAdd and throw on false.
#Resolved

agree and updated, see the other comment.

michaelcfanning · 2021-12-10T20:31:06Z

src/Sarif/PropertiesDictionaryExtensionMethods.cs

@@ -119,6 +115,19 @@ public static class PropertiesDictionaryExtensionMethods
            writer.WriteEndElement(); // Properties
        }

+        private static void SaveSettingNameToDescriptionMap(XmlWriter writer, string key, ConcurrentDictionary<string, string> settingNameToDescriptionMap)


SaveSettingNameToDescriptionMap

What are we doing here? Seems like you found a different issue and are oportunistically fixing it?

[edit] oh, yes, I see the bug, because we 'continue', we didn't call this code.

Why didn't you just move the logical earlier in the method? And avoid injecting four calls to your new method? Is that possible? #Resolved

there were no tests for this and when I add tests this issue show up and blocking my test pass so I should fix it together.

avoid injecting four calls to your new method ---- agree, let me take a look

fixed to only one copy. the method is not needed.

michaelcfanning · 2021-12-10T20:33:30Z

src/Test.UnitTests.Sarif/PropertiesDictionaryTests.cs

+        }
+
+        [Fact]
+        public void PropertiesDictionary_TestCacheDescription()


PropertiesDictionary_TestCacheDescription

Well done! Love seeing these tests relevant to your change, nice work. #Resolved

michaelcfanning

🕐

michaelcfanning · 2022-01-03T23:21:25Z

[Serializable]

Let's remove this, if we're removing the serialization ctor.

In reply to: 1004422690

Refers to: src/Sarif/PropertiesDictionary.cs:18 in 8eb304d. [](commit_id = 8eb304d, deletion_comment = False)

shaopeng-gh · 2022-01-04T03:32:14Z

[Serializable]

fixed.

In reply to: 1004422690

Refers to: src/Sarif/PropertiesDictionary.cs:18 in 8eb304d. [](commit_id = 8eb304d, deletion_comment = False)

michaelcfanning · 2022-01-04T17:50:07Z

src/Test.UnitTests.Sarif/PropertiesDictionaryTests.cs

+        {
+            var properties = new PropertiesDictionary();
+
+            Exception exception = Record.Exception(() =>


Exception

Did you verify that this test actually fails when the fix isn't applied? #Closed

yes this test can repro the error before change the type.

michaelcfanning · 2022-01-04T17:54:08Z

src/Sarif/ExtensionMethods.cs

@@ -585,5 +586,15 @@ internal static void ConsumeElementOfDepth(this XmlReader xmlReader, int endElem
                xmlReader.Read();
            }
        }
+
+        public static void Add<T>(this ConcurrentDictionary<string, T> concurrentDictionary, string key, T value)


ConcurrentDictionary

Why do we need these? Can't we just require our code to cast to IDictionary when appropriate?

What we've done here is effectively add to the public surface area of our SDK API. Do we want to do that?

My suggestion is to remove this and simply cast our internal concurrent dictionary instances to IDictionary. In order not to produce a new public API. #Closed

michaelcfanning · 2022-01-04T18:21:35Z

src/Sarif/ExtensionMethods.cs

@@ -585,5 +586,15 @@ internal static void ConsumeElementOfDepth(this XmlReader xmlReader, int endElem
                xmlReader.Read();
            }
        }
+
+        public static void Add<T>(this ConcurrentDictionary<string, T> concurrentDictionary, string key, T value)


Add

Looking at this more closely, I think what we need to do is to reintroduce IDictionary<string, T> on the TypedPropertiesDictionary class, it is currently not available because it is implemented explicitly in ConcurrentDictionary.

I chased this around a little bit and you may need to change the signatures of multiples types that extend the core typed dictionary. #Closed

…o that it can be used where IDictionary is expected.

michaelcfanning

change TypedPropertiesDictionary to use ConcurrentDictionary

bef3fbe

shaopeng-gh requested review from eddynaka and michaelcfanning as code owners December 9, 2021 09:15

eddynaka reviewed Dec 9, 2021

View reviewed changes

fix SettingNameToDescriptionsMap as well

7377912

fix SaveSettingNameToDescriptionMap

725f201

eddynaka reviewed Dec 10, 2021

View reviewed changes

michaelcfanning reviewed Dec 10, 2021

View reviewed changes

michaelcfanning requested changes Dec 10, 2021

View reviewed changes

shaopeng-gh added 2 commits December 10, 2021 16:17

do not return bool for ConcurrentDictionary.Add()

d43c31e

fix comments

8eb304d

shaopeng-gh changed the title ~~Change TypedPropertiesDictionary to use ConcurrentDictionary~~ Fix: InvalidOperationException when using PropertiesDictionary in a multithreaded application Dec 13, 2021

shaopeng-gh changed the title ~~Fix: InvalidOperationException when using PropertiesDictionary in a multithreaded application~~ BREAKING: Fix InvalidOperationException when using PropertiesDictionary in a multithreaded application and remove [Serializable] for it. Jan 4, 2022

remove [Serializable] from class PropertiesDictionary

bc850ac

michaelcfanning reviewed Jan 4, 2022

View reviewed changes

Add 'Add' and 'Remove' methods from from TypedPropertiesDictionary, s…

649f09e

…o that it can be used where IDictionary is expected.

michaelcfanning enabled auto-merge (squash) January 4, 2022 19:00

shaopeng-gh disabled auto-merge January 4, 2022 19:21

Reset call to Remove in TypedPropertiesDictionary.

74d2451

michaelcfanning approved these changes Jan 4, 2022

View reviewed changes

michaelcfanning enabled auto-merge (squash) January 4, 2022 19:42

michaelcfanning merged commit 2964a2f into main Jan 4, 2022

michaelcfanning deleted the users/shaopeng-gh/dictionary branch January 4, 2022 19:46

shaopeng-gh mentioned this pull request Jan 5, 2022

Remove Add and Remove extension method from ConcurrentDictionary #2418

Merged

eddynaka mentioned this pull request Apr 26, 2022

PropertiesDictionary is throwing ArgumentNullException when comparer is null #2481

Closed

BREAKING: Fix InvalidOperationException when using PropertiesDictionary in a multithreaded application and remove [Serializable] for it. #2415

BREAKING: Fix InvalidOperationException when using PropertiesDictionary in a multithreaded application and remove [Serializable] for it. #2415

Conversation

shaopeng-gh commented Dec 9, 2021 • edited Loading

Description:

Fixes:

Error Details:

Test Case result before Fix:

eddynaka Dec 9, 2021 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddynaka commented Dec 9, 2021 • edited by michaelcfanning Loading

eddynaka Dec 9, 2021 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddynaka Dec 9, 2021 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaopeng-gh commented Dec 9, 2021

shaopeng-gh commented Dec 10, 2021

shaopeng-gh commented Dec 10, 2021

eddynaka Dec 10, 2021 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddynaka Dec 10, 2021 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Dec 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Dec 10, 2021 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Dec 10, 2021 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Dec 10, 2021 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Dec 10, 2021 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

michaelcfanning left a comment

Choose a reason for hiding this comment

michaelcfanning commented Jan 3, 2022 • edited Loading

shaopeng-gh commented Jan 4, 2022

michaelcfanning Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

michaelcfanning Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

michaelcfanning left a comment

Choose a reason for hiding this comment

BREAKING: Fix `InvalidOperationException` when using PropertiesDictionary in a multithreaded application and remove [Serializable] for it. #2415

BREAKING: Fix `InvalidOperationException` when using PropertiesDictionary in a multithreaded application and remove [Serializable] for it. #2415

shaopeng-gh commented Dec 9, 2021 •

edited

Loading

eddynaka Dec 9, 2021 •

edited by michaelcfanning

Loading

eddynaka commented Dec 9, 2021 •

edited by michaelcfanning

Loading

eddynaka Dec 9, 2021 •

edited by shaopeng-gh

Loading

eddynaka Dec 9, 2021 •

edited by shaopeng-gh

Loading

eddynaka Dec 10, 2021 •

edited by shaopeng-gh

Loading

eddynaka Dec 10, 2021 •

edited by shaopeng-gh

Loading

michaelcfanning Dec 10, 2021 •

edited

Loading

michaelcfanning Dec 10, 2021 •

edited by shaopeng-gh

Loading

michaelcfanning Dec 10, 2021 •

edited by shaopeng-gh

Loading

michaelcfanning Dec 10, 2021 •

edited by shaopeng-gh

Loading

michaelcfanning Dec 10, 2021 •

edited by shaopeng-gh

Loading

michaelcfanning commented Jan 3, 2022 •

edited

Loading

michaelcfanning Jan 4, 2022 •

edited

Loading

michaelcfanning Jan 4, 2022 •

edited

Loading

michaelcfanning Jan 4, 2022 •

edited

Loading