java.lang.reflect.Module.WeakSet is not thread-safe

[Peter Levart]

Hi,

While browsing code in java.lang.reflect.Module (I sometimes do that 
just to see how thinks work ;-) I stumbled on the following nested class:

     private static class WeakSet<E> {
         private final ReadWriteLock lock = new ReentrantReadWriteLock();
         private final Lock readLock = lock.readLock();
         private final Lock writeLock = lock.writeLock();

         private final WeakHashMap<E, Boolean> map = new WeakHashMap<>();

         /**
          * Adds the specified element to the set.
          */
         void add(E e) {
             writeLock.lock();
             try {
                 map.put(e, Boolean.TRUE);
             } finally {
                 writeLock.unlock();
             }
         }

         /**
          * Returns {@code true} if this set contains the specified element.
          */
         boolean contains(E e) {
             readLock.lock();
             try {
                 return map.containsKey(e);
             } finally {
                 readLock.unlock();
             }
         }
     }

...while this seems OK from 1st look, it is not. WeakHashMap is not 
thread-safe even for seemingly read-only operations. All its operations 
can mutate internal state in a non-thread-safe way. The simplest way to 
fix this is to use a writeLock for containsKey operation too. But such 
structure does not scale well to multiple threads for frequent lookups.

WeakSet is used in Module to keep track of transient read edges, exports 
and uses of services added dynamically to the module. Modification 
operations are not so performance critical, but lookup operations are.

I propose to add a thread-safe WeakPairMap data structure which 
associates a pair of weakly-reachable keys with a strongly-reachable 
value based on ConcurrentHashMap. Such data structure is 
footprint-friendly, since only a single instance exists for a particular 
purpose, totaling 3 instances for the transient structures serving all 
Modules in the system:

http://cr.openjdk.java.net/~plevart/jdk9-dev/Module.WeakSet.multithreadUnsafe/webrev.01/

So, what do you think?

Regards, Peter