Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Break hangs on HashSet when a loop is formed on entries due to a concurrent operation #28225

Merged
danmoseley merged 3 commits into from
Mar 26, 2018
Merged

Break hangs on HashSet when a loop is formed on entries due to a concurrent operation #28225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a7d2cb2
Break hangs on HashSet when a loop is formed on entries due to a conc…
safern Mar 19, 2018
4325a73
PR Feedback, copy _slots to local ref
safern Mar 20, 2018
6c9a8e6
Get a copy of -slots after IncreaseCapacity changes it
safern Mar 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/System.Collections/src/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,9 @@
<data name="Argument_AddingDuplicate" xml:space="preserve">
<value>An item with the same key has already been added. Key: {0}</value>
</data>
<data name="InvalidOperation_ConcurrentOperationsNotSupported" xml:space="preserve">
<value>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.</value>
</data>
<data name="InvalidOperation_EmptyQueue" xml:space="preserve">
<value>Queue empty.</value>
</data>
Expand Down
98 changes: 72 additions & 26 deletions src/System.Collections/src/System/Collections/Generic/HashSet.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,14 +257,23 @@ public bool Contains(T item)
{
if (_buckets != null)
{
int collisionCount = 0;
int hashCode = InternalGetHashCode(item);
Slot[] slots = _slots;
// see note at "HashSet" level describing why "- 1" appears in for loop
for (int i = _buckets[hashCode % _buckets.Length] - 1; i >= 0; i = _slots[i].next)
for (int i = _buckets[hashCode % _buckets.Length] - 1; i >= 0; i = slots[i].next)
{
if (_slots[i].hashCode == hashCode && _comparer.Equals(_slots[i].value, item))
if (slots[i].hashCode == hashCode && _comparer.Equals(slots[i].value, item))
{
return true;
}

if (collisionCount >= slots.Length)
{
// The chain of entries forms a loop, which means a concurrent update has happened.
throw new InvalidOperationException(SR.InvalidOperation_ConcurrentOperationsNotSupported);
}
collisionCount++;
}
}
// either _buckets is null or wasn't found
Expand Down Expand Up @@ -293,26 +302,28 @@ public bool Remove(T item)
int hashCode = InternalGetHashCode(item);
int bucket = hashCode % _buckets.Length;
int last = -1;
for (int i = _buckets[bucket] - 1; i >= 0; last = i, i = _slots[i].next)
int collisionCount = 0;
Slot[] slots = _slots;
for (int i = _buckets[bucket] - 1; i >= 0; last = i, i = slots[i].next)
{
if (_slots[i].hashCode == hashCode && _comparer.Equals(_slots[i].value, item))
if (slots[i].hashCode == hashCode && _comparer.Equals(slots[i].value, item))
{
if (last < 0)
{
// first iteration; update buckets
_buckets[bucket] = _slots[i].next + 1;
_buckets[bucket] = slots[i].next + 1;
}
else
{
// subsequent iterations; update 'next' pointers
_slots[last].next = _slots[i].next;
slots[last].next = slots[i].next;
}
_slots[i].hashCode = -1;
slots[i].hashCode = -1;
if (RuntimeHelpers.IsReferenceOrContainsReferences<T>())
{
_slots[i].value = default(T);
slots[i].value = default(T);
}
_slots[i].next = _freeList;
slots[i].next = _freeList;

_count--;
_version++;
Expand All @@ -327,6 +338,13 @@ public bool Remove(T item)
}
return true;
}

if (collisionCount >= slots.Length)
{
// The chain of entries forms a loop, which means a concurrent update has happened.
throw new InvalidOperationException(SR.InvalidOperation_ConcurrentOperationsNotSupported);
}
collisionCount++;
}
}
// either _buckets is null or wasn't found
Expand Down Expand Up @@ -1196,35 +1214,44 @@ private bool AddIfNotPresent(T value)

int hashCode = InternalGetHashCode(value);
int bucket = hashCode % _buckets.Length;

for (int i = _buckets[bucket] - 1; i >= 0; i = _slots[i].next)
int collisionCount = 0;
Slot[] slots = _slots;
for (int i = _buckets[bucket] - 1; i >= 0; i = slots[i].next)
{
if (_slots[i].hashCode == hashCode && _comparer.Equals(_slots[i].value, value))
if (slots[i].hashCode == hashCode && _comparer.Equals(slots[i].value, value))
{
return false;
}

if (collisionCount >= slots.Length)
{
// The chain of entries forms a loop, which means a concurrent update has happened.
throw new InvalidOperationException(SR.InvalidOperation_ConcurrentOperationsNotSupported);
}
collisionCount++;
}

int index;
if (_freeList >= 0)
{
index = _freeList;
_freeList = _slots[index].next;
_freeList = slots[index].next;
}
else
{
if (_lastIndex == _slots.Length)
if (_lastIndex == slots.Length)
{
IncreaseCapacity();
Copy link
Copy Markdown
Member

@benaadams benaadams Mar 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to grab slots again here as IncreaseCapacity() will change it. So

IncreaseCapacity();
slots = _slots;

// this will change during resize
slots = _slots;
bucket = hashCode % _buckets.Length;
}
index = _lastIndex;
_lastIndex++;
}
_slots[index].hashCode = hashCode;
_slots[index].value = value;
_slots[index].next = _buckets[bucket] - 1;
slots[index].hashCode = hashCode;
slots[index].value = value;
slots[index].next = _buckets[bucket] - 1;
_buckets[bucket] = index + 1;
_count++;
_version++;
Expand Down Expand Up @@ -1376,13 +1403,22 @@ private int InternalIndexOf(T item)
{
Debug.Assert(_buckets != null, "_buckets was null; callers should check first");

int collisionCount = 0;
int hashCode = InternalGetHashCode(item);
for (int i = _buckets[hashCode % _buckets.Length] - 1; i >= 0; i = _slots[i].next)
Slot[] slots = _slots;
for (int i = _buckets[hashCode % _buckets.Length] - 1; i >= 0; i = slots[i].next)
{
if ((_slots[i].hashCode) == hashCode && _comparer.Equals(_slots[i].value, item))
if ((slots[i].hashCode) == hashCode && _comparer.Equals(slots[i].value, item))
{
return i;
}

if (collisionCount >= slots.Length)
{
// The chain of entries forms a loop, which means a concurrent update has happened.
throw new InvalidOperationException(SR.InvalidOperation_ConcurrentOperationsNotSupported);
}
collisionCount++;
}
// wasn't found
return -1;
Expand Down Expand Up @@ -1500,34 +1536,44 @@ private bool AddOrGetLocation(T value, out int location)

int hashCode = InternalGetHashCode(value);
int bucket = hashCode % _buckets.Length;
for (int i = _buckets[bucket] - 1; i >= 0; i = _slots[i].next)
int collisionCount = 0;
Slot[] slots = _slots;
for (int i = _buckets[bucket] - 1; i >= 0; i = slots[i].next)
{
if (_slots[i].hashCode == hashCode && _comparer.Equals(_slots[i].value, value))
if (slots[i].hashCode == hashCode && _comparer.Equals(slots[i].value, value))
{
location = i;
return false; //already present
}

if (collisionCount >= slots.Length)
{
// The chain of entries forms a loop, which means a concurrent update has happened.
throw new InvalidOperationException(SR.InvalidOperation_ConcurrentOperationsNotSupported);
}
collisionCount++;
}
int index;
if (_freeList >= 0)
{
index = _freeList;
_freeList = _slots[index].next;
_freeList = slots[index].next;
}
else
{
if (_lastIndex == _slots.Length)
if (_lastIndex == slots.Length)
{
IncreaseCapacity();
Copy link
Copy Markdown
Member

@benaadams benaadams Mar 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to grab slots again here as IncreaseCapacity() will change it. So

IncreaseCapacity();
slots = _slots;

Copy link
Copy Markdown
Member Author

@safern safern Mar 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you're right, thanks for the heads up.

// this will change during resize
slots = _slots;
bucket = hashCode % _buckets.Length;
}
index = _lastIndex;
_lastIndex++;
}
_slots[index].hashCode = hashCode;
_slots[index].value = value;
_slots[index].next = _buckets[bucket] - 1;
slots[index].hashCode = hashCode;
slots[index].value = value;
slots[index].next = _buckets[bucket] - 1;
_buckets[bucket] = index + 1;
_count++;
_version++;
Expand Down