BREAKING: Fix InvalidOperationException when using PropertiesDictio…

…nary in a multithreaded application and remove [Serializable] for it. (#2415) * change TypedPropertiesDictionary to use ConcurrentDictionary * fix SettingNameToDescriptionsMap as well * fix SaveSettingNameToDescriptionMap * do not return bool for ConcurrentDictionary.Add() * fix comments * remove [Serializable] from class PropertiesDictionary * Add 'Add' and 'Remove' methods from from TypedPropertiesDictionary, so that it can be used where IDictionary is expected. * Reset call to Remove in TypedPropertiesDictionary. Co-authored-by: Michael C. Fanning <[email protected]>
microsoft · Jan 4, 2022 · 2964a2f · 2964a2f
1 parent b4f73a4
commit 2964a2f
Show file tree

Hide file tree

Showing 6 changed files with 168 additions and 48 deletions.
diff --git a/src/ReleaseHistory.md b/src/ReleaseHistory.md
@@ -1,5 +1,9 @@
 # SARIF Package Release History (SDK, Driver, Converters, and Multitool)
 
+## Unreleased
+
+* BREAKING: Fix `InvalidOperationException` when using PropertiesDictionary in a multithreaded application, and remove `[Serializable]` from it. Now use of BinaryFormatter on it will result in `SerializationException`: Type `PropertiesDictionary` is not marked as serializable. [#2415](https://github.com/microsoft/sarif-sdk/pull/2415)
+
 ## **v2.4.12** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.4.12) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.4.12) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.4.12) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.4.12) | [Multitool Library](https://www.nuget.org/packages/Sarif.Multitool.Library/2.4.12)
 
 * FEATURE: `MultithreadCommandBase` will use cache when hashing is enabled. [#2388](https://github.com/microsoft/sarif-sdk/pull/2388)

diff --git a/src/Sarif/ExtensionMethods.cs b/src/Sarif/ExtensionMethods.cs
@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections;
+using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.Globalization;
 using System.IO;
@@ -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)
+        {
+            ((IDictionary<string, T>)concurrentDictionary).Add(key, value);
+        }
+
+        public static void Remove<T>(this ConcurrentDictionary<string, T> concurrentDictionary, string key)
+        {
+            ((IDictionary<string, T>)concurrentDictionary).Remove(key);
+        }
     }
 }
diff --git a/src/Sarif/PropertiesDictionary.cs b/src/Sarif/PropertiesDictionary.cs
@@ -2,11 +2,11 @@
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
 using System;
+using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.Collections.Immutable;
 using System.ComponentModel;
 using System.IO;
-using System.Runtime.Serialization;
 using System.Xml;
 
 using Newtonsoft.Json;
@@ -15,7 +15,6 @@
 
 namespace Microsoft.CodeAnalysis.Sarif
 {
-    [Serializable]
     [JsonConverter(typeof(TypedPropertiesDictionaryConverter))]
     public class PropertiesDictionary : TypedPropertiesDictionary<object>
     {
@@ -37,11 +36,6 @@ public PropertiesDictionary(
         {
         }
 
-        protected PropertiesDictionary(SerializationInfo info, StreamingContext context)
-            : base(info, context)
-        {
-        }
-
         public string Name { get; set; }
 
         public virtual T GetProperty<T>(PerLanguageOption<T> setting)
@@ -91,7 +85,7 @@ public void SetProperty(IOption setting, object value, bool cacheDescription, bo
 
             if (cacheDescription)
             {
-                SettingNameToDescriptionsMap = SettingNameToDescriptionsMap ?? new Dictionary<string, string>();
+                SettingNameToDescriptionsMap ??= new ConcurrentDictionary<string, string>();
                 SettingNameToDescriptionsMap[setting.Name] = setting.Description;
             }
 

diff --git a/src/Sarif/PropertiesDictionaryExtensionMethods.cs b/src/Sarif/PropertiesDictionaryExtensionMethods.cs
@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections;
+using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.Collections.Specialized;
 using System.ComponentModel;
@@ -27,7 +28,7 @@ public static void SavePropertiesToXmlStream(
             XmlWriter writer,
             XmlWriterSettings settings,
             string name,
-            Dictionary<string, string> settingNameToDescriptionMap)
+            IDictionary<string, string> settingNameToDescriptionMap)
         {
             if (propertyBag == null)
             {
@@ -70,6 +71,16 @@ public static void SavePropertiesToXmlStream(
             {
                 object property = propertyBag[key];
 
+                if (settingNameToDescriptionMap != null)
+                {
+                    string description;
+
+                    if (settingNameToDescriptionMap.TryGetValue(key, out description))
+                    {
+                        writer.WriteComment(description);
+                    }
+                }
+
                 StringSet stringSet = property as StringSet;
                 if (stringSet != null)
                 {
@@ -91,16 +102,6 @@ public static void SavePropertiesToXmlStream(
                     continue;
                 }
 
-                if (settingNameToDescriptionMap != null)
-                {
-                    string description;
-
-                    if (settingNameToDescriptionMap.TryGetValue(key, out description))
-                    {
-                        writer.WriteComment(description);
-                    }
-                }
-
                 writer.WriteStartElement(PROPERTY_ID);
                 writer.WriteAttributeString(KEY_ID, key);
 

diff --git a/src/Sarif/TypedPropertiesDictionary.cs b/src/Sarif/TypedPropertiesDictionary.cs
@@ -2,8 +2,8 @@
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
 using System;
+using System.Collections.Concurrent;
 using System.Collections.Generic;
-using System.Runtime.Serialization;
 
 using Newtonsoft.Json;
 
@@ -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()
     {
         public TypedPropertiesDictionary() : this(null, StringComparer.Ordinal)
         {
@@ -30,13 +30,18 @@ public TypedPropertiesDictionary(PropertiesDictionary initializer, IEqualityComp
             }
         }
 
-        protected TypedPropertiesDictionary(SerializationInfo info, StreamingContext context)
-            : base(info, context)
+        public void Add(string key, T value)
         {
+            ((IDictionary<string, T>)this).Add(key, value);
+        }
+
+        public void Remove(string key)
+        {
+            ((IDictionary<string, T>)this).Remove(key);
         }
 
         [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Usage", "CA2227:CollectionPropertiesShouldBeReadOnly")]
-        protected Dictionary<string, string> SettingNameToDescriptionsMap { get; set; }
+        protected ConcurrentDictionary<string, string> SettingNameToDescriptionsMap { get; set; }
 
         public virtual T GetProperty(PerLanguageOption<T> setting)
         {
@@ -74,7 +79,7 @@ public virtual void SetProperty(IOption setting, T value, bool cacheDescription)
 
             if (cacheDescription)
             {
-                SettingNameToDescriptionsMap = SettingNameToDescriptionsMap ?? new Dictionary<string, string>();
+                SettingNameToDescriptionsMap ??= new ConcurrentDictionary<string, string>();
                 SettingNameToDescriptionsMap[setting.Name] = setting.Description;
             }