Skip to content
This repository was archived by the owner on Nov 1, 2020. It is now read-only.

Port fixes to ConditionalWeakTable #2322

Merged
jkotas merged 1 commit into from
Dec 10, 2016
Merged

Port fixes to ConditionalWeakTable #2322

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
114 changes: 80 additions & 34 deletions src/System.Private.CoreLib/src/System/Runtime/CompilerServices/ConditionalWeakTable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ public sealed class ConditionalWeakTable<TKey, TValue>
#region Constructors
public ConditionalWeakTable()
{
_container = new Container();
_container = new Container(this);
_lock = new Lock();
}
#endregion
Expand Down Expand Up @@ -228,7 +228,7 @@ internal void Clear()
{
using (LockHolder.Hold(_lock))
{
_container = new Container();
_container = new Container(this);
}
}

Expand Down Expand Up @@ -310,8 +310,9 @@ private struct Entry
//
private class Container
{
internal Container()
internal Container(ConditionalWeakTable<TKey, TValue> parent)
{
Debug.Assert(parent != null);
Debug.Assert(IsPowerOfTwo(InitialCapacity));
int size = InitialCapacity;
_buckets = new int[size];
Expand All @@ -320,10 +321,18 @@ internal Container()
_buckets[i] = -1;
}
_entries = new Entry[size];

// Only store the parent after all of the allocations have happened successfully.
// Otherwise, as part of growing or clearing the container, we could end up allocating
// a new Container that fails (OOMs) part way through construction but that gets finalized
// and ends up clearing out some other container present in the associated CWT.
_parent = parent;
}
private Container(int[] buckets, Entry[] entries, int firstFreeEntry)

private Container(ConditionalWeakTable<TKey, TValue> parent, int[] buckets, Entry[] entries, int firstFreeEntry)
{
Debug.Assert(parent != null);
_parent = parent;
_buckets = buckets;
_entries = entries;
_firstFreeEntry = firstFreeEntry;
Expand Down Expand Up @@ -484,23 +493,37 @@ internal Container Resize(int newSize)
{
int hashCode = _entries[entriesIndex].hashCode;
DependentHandle depHnd = _entries[entriesIndex].depHnd;
if (hashCode != -1 && depHnd.IsAllocated && depHnd.GetPrimary() != null)
if (hashCode != -1 && depHnd.IsAllocated)
{
// Entry is used and has not expired. Link it into the appropriate bucket list.
// Note that we have to copy the DependentHandle, since the original copy will be freed
// by the Container's finalizer.
newEntries[newEntriesIndex].hashCode = hashCode;
newEntries[newEntriesIndex].depHnd = depHnd.AllocateCopy();
int bucket = hashCode & (newBuckets.Length - 1);
newEntries[newEntriesIndex].next = newBuckets[bucket];
newBuckets[bucket] = newEntriesIndex;
newEntriesIndex++;
if (depHnd.GetPrimary() != null)
{
// Entry is used and has not expired. Link it into the appropriate bucket list.
newEntries[newEntriesIndex].hashCode = hashCode;
newEntries[newEntriesIndex].depHnd = depHnd;
int bucket = hashCode & (newBuckets.Length - 1);
newEntries[newEntriesIndex].next = newBuckets[bucket];
newBuckets[bucket] = newEntriesIndex;
newEntriesIndex++;
}
else
{
// Pretend the item was removed, so that this container's finalizer
// will clean up this dependent handle.
Volatile.Write(ref _entries[entriesIndex].hashCode, -1);
}
}
}

// Create the new container. We want to transfer the responsibility of freeing the handles from
// the old container to the new container, and also ensure that the new container isn't finalized
// while the old container may still be in use. As such, we store a reference from the old container
// to the new one, which will keep the new container alive as long as the old one is.
var newContainer = new Container(_parent, newBuckets, newEntries, newEntriesIndex);
_oldKeepAlive = newContainer; // once this is set, the old container's finalizer will not free transferred dependent handles

GC.KeepAlive(this); // ensure we don't get finalized while accessing DependentHandles.

return new Container(newBuckets, newEntries, newEntriesIndex);
return newContainer;
}

internal ICollection<TKey> Keys
Expand Down Expand Up @@ -610,14 +633,35 @@ private void VerifyIntegrity()
return;
}

if (_invalid)
//We also skip doing anything if the container is invalid, including if someone
// the container object was allocated but its associated table never set.
if (_invalid || _parent == null)
{
return;
}
Entry[] entries = _entries;

// Make sure anyone sneaking into the table post-resurrection
// gets booted before they can damage the native handle table.
// It's possible that the ConditionalWeakTable could have been resurrected, in which case code could
// be accessing this Container as it's being finalized. We don't support usage after finalization,
// but we also don't want to potentially corrupt state by allowing dependency handles to be used as
// or after they've been freed. To avoid that, if it's at all possible that another thread has a
// reference to this container via the CWT, we remove such a reference and then re-register for
// finalization: the next time around, we can be sure that no references remain to this and we can
// clean up the dependency handles without fear of corruption.
if (!_finalized)
{
_finalized = true;
using (LockHolder.Hold(_parent._lock))
{
if (_parent._container == this)
{
_parent._container = null;
}
}
GC.ReRegisterForFinalize(this); // next time it's finalized, we'll be sure there are no remaining refs
return;
}

Entry[] entries = _entries;
_invalid = true;
_entries = null;
_buckets = null;
Expand All @@ -626,15 +670,26 @@ private void VerifyIntegrity()
{
for (int entriesIndex = 0; entriesIndex < entries.Length; entriesIndex++)
{
entries[entriesIndex].depHnd.Free();
// We need to free handles in two cases:
// - If this container still owns the dependency handle (meaning ownership hasn't been transferred
// to another container that replaced this one), then it should be freed.
// - If this container had the entry removed, then even if in general ownership was transferred to
// another container, removed entries are not, therefore this container must free them.
if (_oldKeepAlive == null || entries[entriesIndex].hashCode == -1)
{
entries[entriesIndex].depHnd.Free();
}
}
}
}

private int[] _buckets; // _buckets[hashcode & (_buckets.Length - 1)] contains index of the first entry in bucket (-1 if empty)
private Entry[] _entries;
private int _firstFreeEntry; // _firstFreeEntry < _entries.Length => table has capacity, entries grow from the bottom of the table.
private bool _invalid; // flag detects if OOM or other background exception threw us out of the lock.
private readonly ConditionalWeakTable<TKey, TValue> _parent; // the ConditionalWeakTable with which this container is associated
private int[] _buckets; // _buckets[hashcode & (_buckets.Length - 1)] contains index of the first entry in bucket (-1 if empty)
private Entry[] _entries; // the table entries containing the stored dependency handles
private int _firstFreeEntry; // _firstFreeEntry < _entries.Length => table has capacity, entries grow from the bottom of the table.
private bool _invalid; // flag detects if OOM or other background exception threw us out of the lock.
private bool _finalized; // set to true when initially finalized
private volatile object _oldKeepAlive; // used to ensure the next allocated container isn't finalized until this one is GC'd
}

private volatile Container _container;
Expand Down Expand Up @@ -717,15 +772,6 @@ public void Free()
}
#endregion

#region Internal Members
internal DependentHandle AllocateCopy()
{
object primary, secondary;
primary = GetPrimaryAndSecondary(out secondary);
return new DependentHandle(primary, secondary);
}
#endregion

#region Private Data Member
private IntPtr _handle;
#endregion
Expand Down