Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Fixes for ConcurrentDictionary for weak memory model hardware #22382

Merged
merged 1 commit into from
Jul 18, 2017

Conversation

m08pvv-zz
Copy link
Contributor

There was possibility to observe partially constructed object (Node) in ConcurrentDictionary on architectures with weak hardware memory model (e.g. ARM).
Volatile.Write protects us from exposing reference to partially constructed Node object.

@kouvel
Copy link
Member

kouvel commented Jul 18, 2017

This looks good to me, but it seems like there may be more places where this would be an issue:

  • GrowTable:
                          newBuckets[newBucketNo] = new Node(current._key, current._value, current._hashcode, newBuckets[newBucketNo]);
    After the array ref is updated with the new array, isn't there still a possibility that TryGetValue would see the new table and node but old values for node fields?
  • Reading from and writing to chain links, for example in TryAddInternal but elsewhere as well:
                                      Node newNode = new Node(node._key, value, hashcode, node._next);
                                      if (prev == null)
                                      {
                                          ...
                                      }
                                      else
                                      {
                                          prev._next = newNode;
                                      }
    It seems possible that TryAdd updates a node with a new node without touching the head node, then TryGetValue sees the new node but old values for its fields.

What do you think?
CC @stephentoub

@kouvel kouvel added this to the 2.1.0 milestone Jul 18, 2017
@m08pvv-zz
Copy link
Contributor Author

@kouvel GrowTable is safe because newBuckets array is exposed with help of _tables = new Tables(newBuckets, newLocks, newCountPerLock); and _tables is declared as volatile, so _tables will be updated after any other preceding memory operations.

_next is also defined as volatile, so reference will be updated safely as well.

@kouvel
Copy link
Member

kouvel commented Jul 18, 2017

Missed the volatile fields, thanks!

@kouvel
Copy link
Member

kouvel commented Jul 18, 2017

@dotnet-bot test Linux x64 Release Build

Looks like a random failure on CentOS:

Unhandled Exception of Type Xunit.Sdk.InRangeException
Message :
Assert.InRange() Failure\r\nRange:  (00:00:00.0000001 - 00:00:00.1412111)\r\nActual: 00:00:00.2264035
Stack Trace :
   at System.Diagnostics.Tests.ActivityTests.DiagnosticSourceStartStop() in /mnt/j/workspace/dotnet_corefx/master/linux-TGroup_netcoreapp+CGroup_Release+AGroup_x64+TestOuter_false_prtest/src/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs:line 432

@stephentoub stephentoub merged commit e3e1322 into dotnet:master Jul 18, 2017
@m08pvv-zz m08pvv-zz deleted the ConcurrentDictionaryFix branch July 19, 2017 09:19
pjanotti pushed a commit to pjanotti/corefx that referenced this pull request Jul 20, 2017
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants