[ 
https://issues.apache.org/jira/browse/AVRO-607?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14387777#comment-14387777
 ] 

Ryan Blue commented on AVRO-607:
--------------------------------

[~davelatham]'s comment is a good excuse to start this thread again. From the 
discussion above, it sounds like building the WeakConcurrentMap is a problem 
because its equals method doesn't override the default and two WeakReferences 
to the same object aren't equal. But, WeakIdentityHashMap creates a subclass of 
WeakReference that overrides equals to avoid that problem and it would be 
appropriate to use an IdentityHashMap for this cache. So I created a version of 
the WeakIdentityHashMap that is backed by a ConcurrentHashMap instead of a 
regular HashMap. Even if the GC removes the weakly-referenced object while the 
ConcurrentHashMap is doing some operation, it either removes all weak 
references or none, so key equality is preserved.

I also caught a couple of places where I think the current implementation 
violates its contract. For example, {{keySet}} gets all of the identity weak 
references and accumulates a set of the real values. As soon as each real value 
is referenced, we know that the weak reference won't disappear. But I think 
there's a small opportunity between reaping the current reference set and 
adding all the referenced objects to the key set for GC to run and remove 
values:

{code:java}
  public Set<K> keySet() {
    reap();
    Set<K> ret = new HashSet<K>();
    for (IdentityWeakReference<K> ref : backingStore.keySet()) {
      // half-way through the loop, GC could run and remove the next referenced 
object
      ret.add(ref.get());
    }
    return Collections.unmodifiableSet(ret);
  }
{code}

I've also added checks so that in {{keySet}} and {{entrySet}}, the 
WeakReference is resolved (which prevents losing the value) and checked to see 
if it is null before adding to the output collection.

I've added [PR #30|https://github.com/apache/avro/pull/30] on github, and I can 
upload a patch if that's preferred.

> SpecificData.getSchema not thread-safe
> --------------------------------------
>
>                 Key: AVRO-607
>                 URL: https://issues.apache.org/jira/browse/AVRO-607
>             Project: Avro
>          Issue Type: Bug
>          Components: java
>    Affects Versions: 1.3.3
>            Reporter: Stephen Tu
>         Attachments: AVRO-607.patch
>
>
> SpecificData.getSchema uses a WeakHashMap to cache schemas, but WeakHashMap 
> is not thread-safe, and the method itself is not synchronized. Seems like 
> this could lead to the data structure getting corrupted. 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to