#2536 Fix race condition in bsonmapper #2538
Conversation
There was a problem hiding this comment.
I think this makes sense, though the whole thing is IMO a little unpleasant.
Probably better than my solution in terms of performance.
One thing I'm still unsure of is whether it is actually safe to concurrently insert into a dictionary while reading from it on another thread, I think there is a chance it fails, if the underlying storage needs to grow, but I'm not familiar enough with the details of Dictionary to say for sure.
| /// <summary> | ||
| /// Unfinished mapping cache between Class/BsonDocument | ||
| /// </summary> | ||
| private readonly Dictionary<Type, EntityMapper> _buildEntities = new Dictionary<Type, EntityMapper>(); |
There was a problem hiding this comment.
how about using ConcurrentDictionary?
There was a problem hiding this comment.
We don't need concurrent access to this because it is only ever used in building the type mapping, which is always happening in the same thread.
| return mapper; | ||
| } | ||
|
|
||
| lock (_entities) |
There was a problem hiding this comment.
You mentioned that is not needed to use ConcurrentDictionary, but we need a lock here in order to access and update its values (for both), are you sure isn't better to use that collection and avoid the lock here?
There was a problem hiding this comment.
In the best case (which happens 99% of the time), we don`t lock. Only initially, when first creating the mapping, we need the lock.
In addidtion to that, this behaviour is the same as the original code before the bugfix that caused this bug
Fixes #2536