CSHARP-5349: Fix discriminator convention inheritance. #1512
Conversation
|
|
||
| if (classMap._isRootClass) | ||
| { | ||
| return StandardDiscriminatorConvention.Hierarchical; |
There was a problem hiding this comment.
I think this is still wrong... there just isn't a test to show that it is.
Consider this scenario: there might be a registered discriminator convention at this level and we're not looking it up.
| if (classMap._isRootClass) | ||
| { | ||
| // in this case auto-register a hierarchical convention for the root class and look it up as usual below | ||
| BsonSerializer.GetOrRegisterDiscriminatorConvention(classMap._classType, StandardDiscriminatorConvention.Hierarchical); |
There was a problem hiding this comment.
Calling GetOrRegisterDiscriminatorConvention instead of RegsiterDiscriminatorConvention is in order to avoid collisions between threads.
There was a problem hiding this comment.
How does it avoid collisions?
There was a problem hiding this comment.
The new GetOrRegisterDiscriminatorConvention method uses the same lock as RegsiterDiscriminatorConvention to handle thread safety.
| @@ -269,6 +269,43 @@ public static object Deserialize(TextReader textReader, Type nominalType, Action | |||
| } | |||
| } | |||
|
|
|||
| internal static IDiscriminatorConvention GetOrRegisterDiscriminatorConvention(Type type, IDiscriminatorConvention discriminatorConvention) | |||
There was a problem hiding this comment.
These don't need to be public.
| __configLock.EnterReadLock(); | ||
| try | ||
| { | ||
| if (__discriminatorConventions.TryGetValue(type, out var registeredConvention)) | ||
| { | ||
| return registeredConvention; | ||
| } | ||
| } | ||
| finally | ||
| { | ||
| __configLock.ExitReadLock(); | ||
| } |
There was a problem hiding this comment.
Is this whole block really needed? Seems a bit redundant to check for a registered convention here and then check again later on below when trying to register.
There was a problem hiding this comment.
It may seem redundant but it's not. All the other methods use the same pattern.
The idea is that after the initial configuration phase is over everything can be done with read lock only, eliminating lock contention between threads.
The second check below is because of multithreading. Between releasing the read lock and acquiring the write lock some other thread might have snuck in and registered the convention.
| if (classMap._isRootClass) | ||
| { | ||
| // in this case auto-register a hierarchical convention for the root class and look it up as usual below | ||
| BsonSerializer.GetOrRegisterDiscriminatorConvention(classMap._classType, StandardDiscriminatorConvention.Hierarchical); |
There was a problem hiding this comment.
Does GetOrRegisterDiscriminatorConvention returns the value we are looking for? Can we return it straight away without additional lookup below?
| { | ||
| return registeredConvention; | ||
| } | ||
| else |
There was a problem hiding this comment.
Minor: this else block if unnecessary as there is return in true path of the check. Can simplify it to reduce the nesting level.
| @@ -269,6 +269,43 @@ public static object Deserialize(TextReader textReader, Type nominalType, Action | |||
| } | |||
| } | |||
|
|
|||
| internal static IDiscriminatorConvention GetOrRegisterDiscriminatorConvention(Type type, IDiscriminatorConvention discriminatorConvention) | |||
There was a problem hiding this comment.
I would suggest to have slightly different method signature to mimic ConcurrentDictionary.GetOrAdd method signature:
internal static IDiscriminatorConvention GetOrAddDiscriminatorConvention(Type type, Func<Type, IDiscriminatorConvention> discriminatorConventionFactory)
No description provided.