Fix race conditions in DatataContractSerialization #31065
Conversation
| #pragma warning disable 0649 | ||
| private XmlNodeReader _xmlNodeReader; | ||
| #pragma warning restore 0649 | ||
|
|
||
| private XmlObjectSerializerReadContext _context; | ||
|
|
||
| private static Dictionary<string, string> s_nsToPrefixTable; | ||
| private static Hashtable s_nsToPrefixTable; |
There was a problem hiding this comment.
I think hash table are safe for concurrent reading and writing https://github.com/dotnet/corefx/issues/29360#issue-318244506.
But it's not using a lock to protect the reads in this function and elsewhere, and unlike Hashtable, Dictionary (and in this case LowLevelDictionary) are not safe for concurrent reading and writing.
There was a problem hiding this comment.
Thanks!
So recap:
s_nsToPrefixTable is safe in this way. The write to it is under a lock, the read is double-checked under the lock.
s_prefixToNsTable gets written to under a lock, while the read is unlocked in LookupNamespace. So the "safeness" depends on when LookupNamespace gets called in relation to GetPrefix. To be on the safe side your cite and Hashtable seems to be correct.
So there should be a comment on why Hashtable is used instead of Dictionary<string, string>, but it's not so obvious.
BTW: the two statics could be readonly.
There was a problem hiding this comment.
While I understand the desire to make sure things are commented so the code is understood, I feel like understanding the behavior characteristics of the main framework collection types should be a given when working with corefx code. Certainly if someone would want to change this they should understand the differences in behavior of HashTable and whatever they are wanting to replace it with before making a change. Otherwise do we need to add a comment to usages of List explaining it's being used because of constant time retrieval by index as opposed to LinkedList which is linear time but can have constant time removal (whereas List is linear time with size with some usage patterns)?
There was a problem hiding this comment.
I think hash table are safe for concurrent reading and writing
They are safe for multiple reader single writer.
There was a problem hiding this comment.
O(1) vs O(n) is obvious, no need for any comment. The relation to List and LinkedList is a bit wrong in this place. It's more generic collection type vs nongeneric type with same perf characteristics.
I feel it's not so obvious that here Hashtable is better suited, hence I'd like to see a hint here.
There was a problem hiding this comment.
They are safe for multiple reader single writer.
@danmosemsft thank's for info!
| @@ -37,20 +37,22 @@ private enum ExtensionDataNodeType | |||
| private int _attributeCount; | |||
| private int _attributeIndex; | |||
|
|
|||
| private static object prefixLock = new object(); | |||
There was a problem hiding this comment.
Nit: prefixLock => s_prefixLock
…h an empty commit to try to make it work
|
Thanks everyone for reviewing this PR 😃 |
* Fix some race conditions in DatataContractSerialization Commit migrated from dotnet/corefx@2b9fa07
Fixes #30651
@mconnew @huanwu @Lxiamail