-
Notifications
You must be signed in to change notification settings - Fork 1.3k
CSHARP-5633: BsonSerializer.IsTypeDiscriminated is not thread-safe #1722
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.
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,7 +39,7 @@ public static class BsonSerializer | |
| private static Dictionary<Type, IIdGenerator> __idGenerators = new Dictionary<Type, IIdGenerator>(); | ||
| private static Dictionary<Type, IDiscriminatorConvention> __discriminatorConventions = new Dictionary<Type, IDiscriminatorConvention>(); | ||
| private static Dictionary<BsonValue, HashSet<Type>> __discriminators = new Dictionary<BsonValue, HashSet<Type>>(); | ||
| private static HashSet<Type> __discriminatedTypes = new HashSet<Type>(); | ||
| private static ConcurrentDictionary<Type, bool> __discriminatedTypes = new (); | ||
| private static BsonSerializerRegistry __serializerRegistry; | ||
| private static TypeMappingSerializationProvider __typeMappingSerializationProvider; | ||
| // ConcurrentDictionary<Type, object> is being used as a concurrent set of Type. The values will always be null. | ||
|
|
@@ -321,8 +321,7 @@ internal static bool IsDiscriminatorConventionRegisteredAtThisLevel(Type type) | |
| /// <returns>True if the type is discriminated.</returns> | ||
| public static bool IsTypeDiscriminated(Type type) | ||
| { | ||
| var typeInfo = type.GetTypeInfo(); | ||
| return typeInfo.IsInterface || __discriminatedTypes.Contains(type); | ||
| return type.GetTypeInfo().IsInterface || __discriminatedTypes.ContainsKey(type); | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -587,7 +586,7 @@ public static void RegisterDiscriminator(Type type, BsonValue discriminator) | |
| // mark all base types as discriminated (so we know that it's worth reading a discriminator) | ||
| for (var baseType = typeInfo.BaseType; baseType != null; baseType = baseType.GetTypeInfo().BaseType) | ||
| { | ||
| __discriminatedTypes.Add(baseType); | ||
| __discriminatedTypes.TryAdd(baseType, true); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it OK that we are ignoring the return value of I understand that we expect it to always return true given the check on line 582. Should we document that we are ignoring the return value on purpose by coding like this:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's safe to ignore the return value of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting... I am a fan of the discard variable. I always prefer code to comments to document something. But it's fine either way. |
||
| } | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetTypeInfois left over from when we used to target a very old .NET Core framework.I think this line can be just:
return type.IsInterface || __discriminatedTypes.ContainsKey(type);There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected.