[GitHub] [lucenenet] NightOwl888 commented on issue #506: IdentityWeakReference should be IDisposable

[GitBox <gi...@apache.org>]

NightOwl888 commented on issue #506:
URL: https://github.com/apache/lucenenet/issues/506#issuecomment-897825129


   @felipepessoto 
   
   Thanks for the report and your insights. I took a closer look and it seems there is more wrong with this than simply not correctly disposing the weak reference. The original declaration in Lucene's `IndexReader` was:
   
   ```java
     private final Set<IndexReader> parentReaders = 
         Collections.synchronizedSet(Collections.newSetFromMap(new WeakHashMap<IndexReader,Boolean>()));
   ```
   
   However, ours is:
   
   ```c#
   private readonly ISet<IdentityWeakReference<IndexReader>> parentReaders = new ConcurrentHashSet<IdentityWeakReference<IndexReader>>();
   ```
   
   Effectively, we have replaced a collection that automatically prunes its weak entries with one that does not.
   
   It seems like the right direction to go here is to factor out the `IdentityWeakReference<T>` class (which is only referenced in `IndexReader`) and replace the collection with `ConditionalWeakTable<TKey, TValue>`, since it would automatically prune and GC the contained keys correctly and it more closely resembles how it was done in Lucene. While there was no "identity" equality comparison in the original Java WeakHashMap, the object that is being compared (`IndexReader`) overrides `GetHashCode()` and `Equals()` to make the identity comparison, so the fact that `ConditionalWeakTable<TKey, TValue>` uses identity comparison is no issue.
   
   Unfortunately, if we do that we need to conditionally compile because the enumerator is not exposed in `ConditionalWeakTable<TKey, TValue>` prior to .NET Stanadard 2.1. So, we will need to fall back to `WeakDictionary<TKey, TValue>` in those cases.
   
   In addition, there is a thread safety issue. The lock of the concurrent set in Java was shared between the `Add` operation and the enumeration, which means we can add items while they are being iterated even though that is not supposed to be allowed.
   
   This report also led me to discover http://www.philosophicalgeek.com/2014/08/14/prefer-weakreferencet-to-weakreference/, which recommends against using the generic `WeakReference` and instead use `WeakReference<T>`, so we can also make some adjustments to [`WeakDictionary<TKey, TValue>`](https://github.com/apache/lucenenet/blob/Lucene.Net_4_8_0_beta00014/src/Lucene.Net/Support/WeakDictionary.cs#L255) in order to make it more robust.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org