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 3 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
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 bool Add<T>(this ConcurrentDictionary<string, T> concurrentDictionary, string key, T value)
Copy link
Member

@michaelcfanning michaelcfanning 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.

Add

Why did you add this? What does the CurrentDictionary IDictionary.Add implementation call into? A non-thread-safe base method? #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.

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.

Copy link
Member

@michaelcfanning michaelcfanning 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.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agree and updated, see the other comment.

{
return concurrentDictionary.TryAdd(key, value);
}

public static bool Remove<T>(this ConcurrentDictionary<string, T> concurrentDictionary, string key)
{
return concurrentDictionary.TryRemove(key, out _);
}
}
}
9 changes: 2 additions & 7 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 Down Expand Up @@ -37,11 +37,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 +86,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
29 changes: 19 additions & 10 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)
ConcurrentDictionary<string, string> settingNameToDescriptionMap)
{
if (propertyBag == null)
{
Expand Down Expand Up @@ -73,33 +74,28 @@ public static void SavePropertiesToXmlStream(
StringSet stringSet = property as StringSet;
if (stringSet != null)
{
SaveSettingNameToDescriptionMap(writer, key, settingNameToDescriptionMap);
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.

SaveSettingNameToDescriptionMap

pls, add tests that cover this change. #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.

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

SaveSet(writer, stringSet, key);
continue;
}

IntegerSet integerSet = property as IntegerSet;
if (integerSet != null)
{
SaveSettingNameToDescriptionMap(writer, key, settingNameToDescriptionMap);
SaveSet(writer, integerSet, key);
continue;
}

IDictionary pb = property as IDictionary;
if (pb != null)
{
SaveSettingNameToDescriptionMap(writer, key, settingNameToDescriptionMap);
((IDictionary)pb).SavePropertiesToXmlStream(writer, settings, key, settingNameToDescriptionMap);
continue;
}

if (settingNameToDescriptionMap != null)
{
string description;

if (settingNameToDescriptionMap.TryGetValue(key, out description))
{
writer.WriteComment(description);
}
}
SaveSettingNameToDescriptionMap(writer, key, settingNameToDescriptionMap);

writer.WriteStartElement(PROPERTY_ID);
writer.WriteAttributeString(KEY_ID, key);
Expand All @@ -119,6 +115,19 @@ public static void SavePropertiesToXmlStream(
writer.WriteEndElement(); // Properties
}

private static void SaveSettingNameToDescriptionMap(XmlWriter writer, string key, ConcurrentDictionary<string, string> settingNameToDescriptionMap)
Copy link
Member

@michaelcfanning michaelcfanning 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.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

{
if (settingNameToDescriptionMap != null)
{
string description;

if (settingNameToDescriptionMap.TryGetValue(key, out description))
{
writer.WriteComment(description);
}
}
}

private static string NormalizeTypeName(string typeName)
{
// This helper currently assumes that namespaces in
Expand Down
13 changes: 4 additions & 9 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,8 @@ public TypedPropertiesDictionary(PropertiesDictionary initializer, IEqualityComp
}
}

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

[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 +69,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