Fix for #506 - Prune dead elements from IndexReader.parentReaders collection when they go out of scope

src/Lucene.Net/Index/IndexReader.cs

@@ -100,7 +100,14 @@ public interface IReaderClosedListener
 
         private readonly ISet<IReaderClosedListener> readerClosedListeners = new JCG.LinkedHashSet<IReaderClosedListener>().AsConcurrent();
 
-        private readonly ISet<IdentityWeakReference<IndexReader>> parentReaders = new ConcurrentHashSet<IdentityWeakReference<IndexReader>>();
+#if FEATURE_CONDITIONALWEAKTABLE_ENUMERATOR
+        private readonly ConditionalWeakTable<IndexReader, object> parentReaders = new ConditionalWeakTable<IndexReader, object>();
+#else
+        private readonly WeakDictionary<IndexReader, object> parentReaders = new WeakDictionary<IndexReader, object>();
+#endif
+        // LUCENENET specific - since neither WeakDictionary nor ConditionalWeakTable synchronize
+        // on the enumerator, we need to do external synchronization to make them threadsafe.
+        private readonly object parentReadersLock = new object();
 
         /// <summary>
         /// Expert: adds a <see cref="IReaderClosedListener"/>.  The
@@ -136,7 +143,14 @@ public void RemoveReaderClosedListener(IReaderClosedListener listener)
         public void RegisterParentReader(IndexReader reader)
         {
             EnsureOpen();
-            parentReaders.Add(new IdentityWeakReference<IndexReader>(reader));
+            // LUCENENET specific - since neither WeakDictionary nor ConditionalWeakTable synchronize
+            // on the enumerator, we need to do external synchronization to make them threadsafe.
+            lock (parentReadersLock)
+                // LUCENENET: Since there is a set Add operation (unique) in Lucene, the equivalent
+                // operation in .NET is AddOrUpdate, which effectively does nothing if the key exists.
+                // Null is passed as a value, since it is not used anyway and .NET doesn't have a boolean
+                // reference type.
+                parentReaders.AddOrUpdate(key: reader, value: null);
         }
 
         private void NotifyReaderClosedListeners(Exception th)
@@ -167,15 +181,18 @@ private void NotifyReaderClosedListeners(Exception th)
 
         private void ReportCloseToParentReaders()
         {
-            lock (parentReaders) // LUCENENET: This does not actually synchronize the set, it only ensures this method can only be entered by 1 thread
+            // LUCENENET specific - since neither WeakDictionary nor ConditionalWeakTable synchronize
+            // on the enumerator, we need to do external synchronization to make them threadsafe.
+            lock (parentReadersLock)
             {
-                foreach (IdentityWeakReference<IndexReader> parent in parentReaders)
+                foreach (var kvp in parentReaders)
                 {
-                    //Using weak references
-                    IndexReader target = parent.Target;
+                    IndexReader target = kvp.Key;
 
+                    // LUCENENET: This probably can't happen, but we are being defensive to avoid exceptions
                     if (target != null)
                     {
+                        //Using weak references
                         target.closedByChild = true;
                         // cross memory barrier by a fake write:
                         target.refCount.AddAndGet(0);

src/Lucene.Net/Support/WeakDictionary.cs

@@ -19,12 +19,11 @@
  *
 */
 
-
 #if !FEATURE_CONDITIONALWEAKTABLE_ENUMERATOR
 using System;
 using System.Collections;
+using System.Collections.Concurrent;
 using System.Collections.Generic;
-using System.Linq;
 using JCG = J2N.Collections.Generic;
 
 namespace Lucene.Net.Support
@@ -51,7 +50,9 @@ public WeakDictionary(IEnumerable<KeyValuePair<TKey, TValue>> otherDictionary)
 
         private WeakDictionary(int initialCapacity, IEnumerable<KeyValuePair<TKey, TValue>> otherDict)
         {
-            _hm = new JCG.Dictionary<WeakKey<TKey>, TValue>(initialCapacity);
+            // LUCENENET specific - using ConcurrentDictionary here not because we need concurrency, but because
+            // it allows removing items while iterating forward.
+            _hm = new ConcurrentDictionary<WeakKey<TKey>, TValue>(concurrencyLevel: 10, capacity: initialCapacity);
             foreach (var kvp in otherDict)
             {
                 _hm.Add(new WeakKey<TKey>(kvp.Key), kvp.Value);
@@ -65,12 +66,11 @@ private WeakDictionary(int initialCapacity, IEnumerable<KeyValuePair<TKey, TValu
         private void Clean()
         {
             if (_hm.Count == 0) return;
-            var newHm = new JCG.Dictionary<WeakKey<TKey>, TValue>(_hm.Count);
-            foreach (var entry in _hm.Where(x => x.Key != null && x.Key.IsAlive))
+            foreach (var kvp in _hm)
             {
-                newHm.Add(entry.Key, entry.Value);
+                if (!kvp.Key.TryGetTarget(out TKey _))
+                    _hm.Remove(kvp.Key);
             }
-            _hm = newHm;
         }
 
         private void CleanIfNeeded()
@@ -85,9 +85,12 @@ private void CleanIfNeeded()
 
         public IEnumerator<KeyValuePair<TKey, TValue>> GetEnumerator()
         {
-            foreach (var kvp in _hm.Where(x => x.Key.IsAlive))
+            foreach (var kvp in _hm)
             {
-                yield return new KeyValuePair<TKey, TValue>(kvp.Key.Target, kvp.Value);
+                if (kvp.Key.TryGetTarget(out TKey key))
+                {
+                    yield return new KeyValuePair<TKey, TValue>(key, kvp.Value);
+                }
             }
         }
 
@@ -185,7 +188,7 @@ void ICollection<KeyValuePair<TKey, TValue>>.CopyTo(KeyValuePair<TKey, TValue>[]
             throw new NotSupportedException();
         }
 
-#region KeyCollection
+        #region Nested Class: KeyCollection
 
         private class KeyCollection : ICollection<TKey>
         {
@@ -200,7 +203,8 @@ public IEnumerator<TKey> GetEnumerator()
             {
                 foreach (var key in _internalDict.Keys)
                 {
-                    yield return key.Target;
+                    if (key.TryGetTarget(out TKey target))
+                        yield return target;
                 }
             }
 
@@ -243,7 +247,7 @@ bool ICollection<TKey>.Remove(TKey item)
 #endregion Explicit Interface Definitions
         }
 
-#endregion KeyCollection
+        #endregion Nested Class: KeyCollection
 
         /// <summary>
         /// A weak reference wrapper for the hashtable keys. Whenever a key\value pair
@@ -252,16 +256,16 @@ bool ICollection<TKey>.Remove(TKey item)
         /// </summary>
         private class WeakKey<T> where T : class
         {
-            private readonly WeakReference reference;
+            private readonly WeakReference<T> reference;
             private readonly int hashCode;
 
             public WeakKey(T key)
             {
                 if (key is null)
                     throw new ArgumentNullException(nameof(key));
 
-                hashCode = key.GetHashCode();
-                reference = new WeakReference(key);
+                hashCode = key is null ? 0 : key.GetHashCode();
+                reference = new WeakReference<T>(key);
             }
 
             public override int GetHashCode()
@@ -271,25 +275,20 @@ public override int GetHashCode()
 
             public override bool Equals(object obj)
             {
-                if (!reference.IsAlive || obj is null) return false;
-
-                if (object.ReferenceEquals(this, obj))
-                {
-                    return true;
-                }
-
+                if (obj is null) return false;
                 if (obj is WeakKey<T> other)
                 {
-                    var referenceTarget = reference.Target; // Careful: can be null in the mean time...
-                    return referenceTarget != null && referenceTarget.Equals(other.Target);
+                    bool gotThis = this.TryGetTarget(out T thisTarget), gotOther = other.TryGetTarget(out T otherTarget);
+                    if (gotThis && gotOther && thisTarget.Equals(otherTarget))
+                        return true;
+                    else if (gotThis == false && gotOther == false)
+                        return true;
                 }
 
                 return false;
             }
 
-            public T Target => (T)reference.Target;
-
-            public bool IsAlive => reference.IsAlive;
+            public bool TryGetTarget(out T target) => reference.TryGetTarget(out target);
         }
     }
 }