Prevent concurrent use corruption from causing infinite loops #16991
Conversation
src/mscorlib/Resources/Strings.resx
Outdated
| @@ -2539,6 +2539,9 @@ | |||
| <data name="InvalidOperation_CollectionCorrupted" xml:space="preserve"> | |||
| <value>A prior operation on this collection was interrupted by an exception. Collection's state is no longer trusted.</value> | |||
| </data> | |||
| <data name="InvalidOperation_ConcurrentOperationsNotSupported" xml:space="preserve"> | |||
| <value>Concurrent operations are not supported on non-concurrent collections. A concurrent operation was performed on this collection and corrupted its state. Collection's state is no longer correct.</value> | |||
There was a problem hiding this comment.
Nit: "Collection's state" => "The collection's state"
There was a problem hiding this comment.
Concurrent reads are supported right? Maybe "Operations that change this collection must have exclusive access" or something like that?
There was a problem hiding this comment.
Now says:
Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.
How's that?
| // The chain of entires forms a loop; which means a concurrent update has happened. | ||
| // Break out of the loop and throw; rather than looping forever. | ||
| ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported(); | ||
| } |
There was a problem hiding this comment.
What kind of measurable impact, if any, does this have on TryGetValue?
There was a problem hiding this comment.
It shouldn't impact items that haven't collided (as they don't loop); hoping it will be minimal; but will have a look.
On the flip side, what's the alternative?
There was a problem hiding this comment.
On the flip side, what's the alternative?
If it's very negatively impactful, not doing it.
| @@ -382,6 +383,13 @@ private int FindEntry(TKey key) | |||
| } | |||
|
|
|||
| i = entries[i].next; | |||
| if (loops >= entries.Length) | |||
| { | |||
| // The chain of entires forms a loop; which means a concurrent update has happened. | |||
There was a problem hiding this comment.
Nit: search and replace "entires" => "entries"
| if (loops >= entries.Length) | ||
| { | ||
| // The chain of entires forms a loop; which means a concurrent update has happened. | ||
| // Break out of the loop and throw; rather than looping forever. |
There was a problem hiding this comment.
Nit: ';' => ',' (same in the other copies of this)
| // Break out of the loop and throw; rather than looping forever. | ||
| ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported(); | ||
| } | ||
| loops++; |
There was a problem hiding this comment.
What about using the same name collisioncount throughout?
|
Note that the original idea from Feng was on HashSet not Dictionary<K, V>. I think it was reasonable that this issue is more likely on Dictionary since it is used more commonly. Still it may be worth determining whether we apply this change to HashSet. |
For
DictionaryResolves: https://github.com/dotnet/corefx/issues/28123
/cc @vancem @jkotas @danmosemsft @stephentoub