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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 8 commits into from
Jan 4, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bef3fbe
change TypedPropertiesDictionary to use ConcurrentDictionary
shaopeng-gh Dec 9, 2021
7377912
fix SettingNameToDescriptionsMap as well
shaopeng-gh Dec 10, 2021
725f201
fix SaveSettingNameToDescriptionMap
shaopeng-gh Dec 10, 2021
d43c31e
do not return bool for ConcurrentDictionary.Add()
shaopeng-gh Dec 11, 2021
8eb304d
fix comments
shaopeng-gh Dec 12, 2021
bc850ac
remove [Serializable] from class PropertiesDictionary
shaopeng-gh Jan 4, 2022
649f09e
Add 'Add' and 'Remove' methods from from TypedPropertiesDictionary, s…
michaelcfanning Jan 4, 2022
74d2451
Reset call to Remove in TypedPropertiesDictionary.
michaelcfanning Jan 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/ReleaseHistory.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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)
Expand Down
11 changes: 11 additions & 0 deletions src/Sarif/ExtensionMethods.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Collections;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Globalization;
using System.IO;
Expand Down Expand Up @@ -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)
Copy link
Member

@michaelcfanning michaelcfanning Jan 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@michaelcfanning michaelcfanning Jan 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
((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);
}
}
}
10 changes: 2 additions & 8 deletions src/Sarif/PropertiesDictionary.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -15,7 +15,6 @@

namespace Microsoft.CodeAnalysis.Sarif
{
[Serializable]
[JsonConverter(typeof(TypedPropertiesDictionaryConverter))]
public class PropertiesDictionary : TypedPropertiesDictionary<object>
{
Expand All @@ -37,11 +36,6 @@ public PropertiesDictionary(
{
}

protected PropertiesDictionary(SerializationInfo info, StreamingContext context)
Copy link
Member

@michaelcfanning michaelcfanning Dec 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PropertiesDictionary

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

: base(info, context)
{
}
Copy link
Collaborator

@eddynaka eddynaka Dec 9, 2021 •
edited by michaelcfanning
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, thanks

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the file written are the same


public string Name { get; set; }

public virtual T GetProperty<T>(PerLanguageOption<T> setting)
Expand Down Expand Up @@ -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;
}

Expand Down
23 changes: 12 additions & 11 deletions src/Sarif/PropertiesDictionaryExtensionMethods.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Collections;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Collections.Specialized;
using System.ComponentModel;
Expand All @@ -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)
{
Expand Down Expand Up @@ -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)
{
Expand All @@ -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);

Expand Down
17 changes: 11 additions & 6 deletions src/Sarif/TypedPropertiesDictionary.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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()
Copy link
Collaborator

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done both, thanks

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also added the ReleaseHistory for the other 2 PRs.

{
public TypedPropertiesDictionary() : this(null, StringComparer.Ordinal)
{
Expand All @@ -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)
{
Expand Down Expand Up @@ -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;
}

Expand Down
Loading