ben-manes / concurrentlinkedhashmap

A ConcurrentLinkedHashMap for Java
Apache License 2.0
470 stars 113 forks source link

NPE on keySet() #5

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Email from Greg Luck (ehcache):
-------------------------------

I found a problem while running testConcurrentReadWriteRemove in 
CacheTest. The issues below happen with Second Change and FIFO. I have not 
tested LRU.

NPE when multiple threads concurrently call clear()

I  was getting NPE when multiple threads were calling clear() at the same 
time.

java.lang.NullPointerException
at net.sf.ehcache.concurrent.ConcurrentLinkedHashMap
$EntryIteratorAdapter.next(ConcurrentLinkedHashMap.java:695)
at net.sf.ehcache.concurrent.ConcurrentLinkedHashMap
$EntryIteratorAdapter.next(ConcurrentLinkedHashMap.java:670)
at java.util.AbstractMap$1$1.next(AbstractMap.java:383)
at net.sf.ehcache.concurrent.ConcurrentLinkedHashMap.clear
(ConcurrentLinkedHashMap.java:174)
at net.sf.ehcache.store.MemoryStore.clear(MemoryStore.java:209)
at net.sf.ehcache.store.MemoryStore.removeAll(MemoryStore.java:202)
at net.sf.ehcache.Cache.removeAll(Cache.java:1441)
at net.sf.ehcache.Cache.removeAll(Cache.java:1426)
at net.sf.ehcache.CacheTest$1.execute(CacheTest.java:1778)
at net.sf.ehcache.AbstractCacheTest$1.run(AbstractCacheTest.java:158)
java.lang.NullPointerException

I changed Entry to check for nulls. 

/**
         * {@inheritDoc}
         */
        public Entry<K, V> next() {
            current = iterator.next();

            K key = current.getKey();
            Node<K, V> currentNode = current.getValue();
            V value;
            if (currentNode != null) {
                value = currentNode.getValue();
            } else {
                value = null;
            }
            return new SimpleEntry<K, V>(key, value);
        }

ArrayIndexOutOfBoundsException on toArray()

This one happens when I do map.keySet().

SEVERE: Throwable java.lang.ArrayIndexOutOfBoundsException: 3 3
java.lang.ArrayIndexOutOfBoundsException: 3
at java.util.AbstractCollection.toArray(AbstractCollection.java:126)
at net.sf.ehcache.store.MemoryStore.getKeyArray(MemoryStore.java:296)
at net.sf.ehcache.Cache.getKeys(Cache.java:1087)
at net.sf.ehcache.CacheTest$5.execute(CacheTest.java:1802)
at net.sf.ehcache.AbstractCacheTest$1.run(AbstractCacheTest.java:158)
Feb 15, 2009 7:03:00 PM net.sf.ehcache.AbstractCacheTest$1 run
SEVERE: Throwable java.lang.ArrayIndexOutOfBoundsException: 422 422
java.lang.ArrayIndexOutOfBoundsException: 422
at java.util.AbstractCollection.toArray(AbstractCollection.java:126)
at net.sf.ehcache.store.MemoryStore.getKeyArray(MemoryStore.java:296)
at net.sf.ehcache.Cache.getKeys(Cache.java:1087)
at net.sf.ehcache.CacheTest$5.execute(CacheTest.java:1802)
at net.sf.ehcache.AbstractCacheTest$1.run(AbstractCacheTest.java:158)
Feb 15, 2009 7:03:01 PM net.sf.ehcache.AbstractCacheTest$1 run

ConcurrentHashMap overrides this. CLHM does not which I think is the 
problem.

Original issue reported on code.google.com by Ben.Manes@gmail.com on 26 Feb 2009 at 2:04

GoogleCodeExporter commented 9 years ago
Looks like I forgot to override keySet() to delegate to CHM's method. The 
AbstractMap retrieves them in an inefficient, but generic manner.

The node within the CHM should never be null. The entry should be removed, the 
node's value null'd out for GC, and the linkage lazily cleaned up. I need to 
investigate this and duplicate in my unit tests.

Original comment by Ben.Manes@gmail.com on 26 Feb 2009 at 2:08

GoogleCodeExporter commented 9 years ago
While nulling out the value occurs after that entry is removed, I did not 
consider 
the effect of concurrent removals. Since readers see a snapshot of the entry 
set in 
the CHM's segment, a subsequent removal does not drop the entry from the links 
seen 
by previous readers. This means that when I remove the entry and null out the 
value 
to mark it as dead, other readers will see the change. I had forgotten the core 
design idea behind CHM!

This is fairly easy to fix for entrySet()/values(). The iterator can simply 
skip 
ahead and peek at the next valid entry. For keySet(), the CHM's should just be 
returned, as no unwrapping of the value is needed.

Original comment by Ben.Manes@gmail.com on 27 Feb 2009 at 5:28

GoogleCodeExporter commented 9 years ago

Original comment by Ben.Manes@gmail.com on 1 Mar 2009 at 8:58

GoogleCodeExporter commented 9 years ago
Resolved in CL-206.

Original comment by Ben.Manes@gmail.com on 7 Mar 2009 at 8:32

GoogleCodeExporter commented 9 years ago

Original comment by Ben.Manes@gmail.com on 7 Mar 2009 at 8:38

GoogleCodeExporter commented 9 years ago

Original comment by Ben.Manes@gmail.com on 7 Mar 2009 at 8:38