Skip to content

Commit

Permalink
ISPN-7080 NPE in CacheNotifierImpl by LIRS eviction listener
Browse files Browse the repository at this point in the history 
* Just remove NONRESIDENT node immediately
* Cleaned up test to properly check for count
  • Loading branch information
@wburns @danberindei
wburns authored and danberindei committed Nov 2, 2016
1 parent 1246324 commit e3dc640
Show file tree
Hide file tree
Showing 2 changed files with 78 additions and 63 deletions.
99 changes: 49 additions & 50 deletions ...finispan/commons/util/concurrent/jdk8backported/BoundedEquivalentConcurrentHashMapV8.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -713,7 +713,7 @@ public Collection<Node<K, V>> findIfEntriesNeedEvicting() {
}

if (removed) {
V value = map.replaceNode(node.key, null, null, true);
V value = map.replaceNode(node.key, null, null, false, true);
if (value != null) {
evictedEntries.add(node);
decCreate += sizeCalculator.calculateSize(node.key, value);
Expand Down Expand Up @@ -869,8 +869,6 @@ static final class LIRSEvictionPolicy<K, V> implements EvictionPolicy<K, V> {
private final AtomicReference<SizeAndEvicting> currentSize = new AtomicReference<>(
new SizeAndEvicting(0, 0));

final ThreadLocal<Collection<LIRSNode<K, V>>> nodesToEvictTL = new ThreadLocal<>();

public LIRSEvictionPolicy(BoundedEquivalentConcurrentHashMapV8<K, V> map, long maxSize) {
this.map = map;
this.maximumSize = maxSize;
Expand Down Expand Up @@ -1127,6 +1125,7 @@ Object[] pruneIncludingLIR() {
}
DequeNode<LIRSNode<K, V>> removedStackNode = (DequeNode<LIRSNode<K, V>>) nodeDetails[0];
removedLIR = (LIRSNode<K, V>) nodeDetails[1];
boolean shouldRemove = false;
synchronized (removedLIR) {
if (removedStackNode != removedLIR.stackNode) {
continue;
Expand All @@ -1139,13 +1138,10 @@ Object[] pruneIncludingLIR() {
case HIR_NONRESIDENT:
// Non resident was already evicted and now it is no longer in the
// queue or the stack so it is effectively gone - however we want to
// remove the now null node
Collection<LIRSNode<K, V>> nodesToEvict = nodesToEvictTL.get();
if (nodesToEvict == null) {
nodesToEvict = new ArrayList<>();
nodesToEvictTL.set(nodesToEvict);
}
nodesToEvict.add(removedLIR);
// remove the now null node - must be done outside of node synchronized block
// to prevent dead lock
removedLIR.setState(Recency.EVICTING);
shouldRemove = true;
case HIR_RESIDENT:
// Leave it in the queue if it was a resident
removedLIR.setStackNode(null);
Expand All @@ -1158,6 +1154,11 @@ Object[] pruneIncludingLIR() {
break;
}
}
if (shouldRemove) {
// We skip notification since we already were notified of removal - but only remove if the value
// is still NULL_VALUE
map.replaceNode(removedLIR.getKey(), null, null, true, false);
}
}
}

Expand Down Expand Up @@ -1431,22 +1432,10 @@ public Collection<Node<K, V>> findIfEntriesNeedEvicting() {
break;
}
}
// If this is non null it is also non empty
Collection<LIRSNode<K, V>> tlEvicted = nodesToEvictTL.get();
if (tlEvicted == null) {
tlEvicted = Collections.emptyList();
} else {
nodesToEvictTL.remove();
}
if (evictCount != 0 || !tlEvicted.isEmpty()) {
if (evictCount != 0) {
@SuppressWarnings("unchecked")
LIRSNode<K, V>[] queueContents = new LIRSNode[evictCount + tlEvicted.size()];
Iterator<LIRSNode<K, V>> tlIterator = tlEvicted.iterator();
LIRSNode<K, V>[] queueContents = new LIRSNode[evictCount];
int offset = 0;
while (tlIterator.hasNext()) {
queueContents[evictCount + offset] = tlIterator.next();
offset++;
}
int evictedValues = evictCount;
int decEvict = evictCount;
Object[] hirDetails = new Object[2];
Expand Down Expand Up @@ -1547,7 +1536,7 @@ else if (f.hash == MOVED)
synchronized (evict) {
if (evict.state == Recency.EVICTING) {
evict.setState(Recency.EVICTED);
V prevValue = map.replaceNode(evict.getKey(), null, null, true);
V prevValue = map.replaceNode(evict.getKey(), null, null, false, true);
removedNodes.add(new Node<>(-1, null, evict.getKey(),
prevValue, null));
} else if (evict.state == Recency.HIR_NONRESIDENT) {
Expand Down Expand Up @@ -2692,7 +2681,7 @@ public V remove(Object key) {
}

final V replaceNode(Object key, V value, Object cv) {
return replaceNode(key, value, cv, false);
return replaceNode(key, value, cv, false, false);
}

@SuppressWarnings({ "rawtypes", "unchecked" })
Expand All @@ -2709,7 +2698,7 @@ private void notifyListenerOfRemoval(Node removedNode, boolean isEvict) {
* Replaces node value with v, conditional upon match of cv if
* non-null. If resulting value is null, delete.
*/
final V replaceNode(Object key, V value, Object cv, boolean isEvict) {
final V replaceNode(Object key, V value, Object cv, boolean skipListener, boolean isEvict) {
int hash = spread(keyEq.hashCode(key)); // EQUIVALENCE_MOD
for (Node<K,V>[] tab = table;;) {
Node<K,V> f; int n, i, fh;
Expand All @@ -2736,27 +2725,33 @@ else if ((fh = f.hash) == MOVED)
oldVal = ev;
if (value != null) {
e.val = value;
if (oldVal == null) {
evictionPolicy.onEntryMiss(e, value);
} else {
evictionPolicy.onEntryHitWrite(e, value);
if (!skipListener) {
if (oldVal == null) {
evictionPolicy.onEntryMiss(e, value);
} else {
evictionPolicy.onEntryHitWrite(e, value);
}
}
}
else if (pred != null) {
if (!isEvict) {
evictionPolicy.onEntryRemove(e);
}
if (oldVal != null) {
notifyListenerOfRemoval(e, isEvict);
if (!skipListener) {
if (!isEvict) {
evictionPolicy.onEntryRemove(e);
}
if (oldVal != null) {
notifyListenerOfRemoval(e, isEvict);
}
}
pred.next = e.next;
}
else {
if (!isEvict) {
evictionPolicy.onEntryRemove(e);
}
if (oldVal != null) {
notifyListenerOfRemoval(e, isEvict);
if (!skipListener) {
if (!isEvict) {
evictionPolicy.onEntryRemove(e);
}
if (oldVal != null) {
notifyListenerOfRemoval(e, isEvict);
}
}
setTabAt(tab, i, e.next);
}
Expand All @@ -2780,21 +2775,25 @@ else if (f instanceof TreeBin) {
oldVal = pv;
if (value != null) {
p.val = value;
if (oldVal == null) {
evictionPolicy.onEntryMiss(p, value);
} else {
evictionPolicy.onEntryHitWrite(p, value);
if (!skipListener) {
if (oldVal == null) {
evictionPolicy.onEntryMiss(p, value);
} else {
evictionPolicy.onEntryHitWrite(p, value);
}
}
}
else {
if (t.removeTreeNode(p)) {
setTabAt(tab, i, untreeify(t.first)); // EQUIVALENCE_MOD
}
if (!isEvict) {
evictionPolicy.onEntryRemove(p);
}
if (pv != null) {
notifyListenerOfRemoval(p, isEvict);
if (!skipListener) {
if (!isEvict) {
evictionPolicy.onEntryRemove(p);
}
if (pv != null) {
notifyListenerOfRemoval(p, isEvict);
}
}
}
}
Expand Down
42 changes: 29 additions & 13 deletions core/src/test/java/org/infinispan/eviction/impl/BaseEvictionFunctionalTest.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
package org.infinispan.eviction.impl;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Random;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
Expand All @@ -16,10 +20,12 @@
import org.infinispan.test.fwk.TestCacheManagerFactory;
import org.testng.annotations.Test;

import static org.testng.AssertJUnit.assertEquals;

@Test(groups = "functional", testName = "eviction.BaseEvictionFunctionalTest")
public abstract class BaseEvictionFunctionalTest extends SingleCacheManagerTest {

private static final int CACHE_SIZE=128;
private static final int CACHE_SIZE = 64;

private EvictionListener evictionListener;

Expand All @@ -43,18 +49,28 @@ protected EmbeddedCacheManager createCacheManager() throws Exception {
}

public void testSimpleEvictionMaxEntries() throws Exception {
for (int i = 0; i < CACHE_SIZE*2; i++) {
cache.put("key-" + (i + 1), "value-" + (i + 1), 1, TimeUnit.MINUTES);
for (int i = 0; i < CACHE_SIZE * 2; i++) {
cache.put("key-" + (i + 1), "value-" + (i + 1));
}
Thread.sleep(1000); // sleep long enough to allow the thread to wake-up
assert CACHE_SIZE >= cache.size() : "cache size too big: " + cache.size();
assert CACHE_SIZE == evictionListener.getEvictedEvents() : "eviction events count should be same with case size: " + evictionListener.getEvictedEvents();
assertEquals("cache size too big: " + cache.size(), CACHE_SIZE, cache.size());
assertEquals("eviction events count should be same with case size: " + evictionListener.getEvictedEvents(),
CACHE_SIZE, evictionListener.getEvictedEvents().size());

for (int i = 0; i < CACHE_SIZE; i++) {
cache.put("key-" + (i + 1), "value-" + (i + 1), 1, TimeUnit.MINUTES);
cache.put("key-" + (i + 1), "value-" + (i + 1));
}
assertEquals(CACHE_SIZE, cache.size());
int expectedEvictions;
if (getEvictionStrategy() == EvictionStrategy.LIRS) {
// Eviction count will be Size + (Size * .05) rounded up (since the first elements will be in resident blocks
// so they won't cause evictions to occur
expectedEvictions = (int) Math.ceil(CACHE_SIZE + (CACHE_SIZE * .05));
} else {
// Otherwise is LRU and that will evict on each write
expectedEvictions = CACHE_SIZE * 2;
}
Thread.sleep(1000); // sleep long enough to allow the thread to wake-up
assert CACHE_SIZE >= cache.size() : "cache size too big: " + cache.size();
assertEquals("eviction events count should be same with case size: " + evictionListener.getEvictedEvents(),
expectedEvictions, evictionListener.getEvictedEvents().size());
}

public void testSimpleExpirationMaxIdle() throws Exception {
Expand Down Expand Up @@ -131,7 +147,7 @@ public void run() {
@Listener
public static class EvictionListener {

private AtomicLong evictedEvents = new AtomicLong();
private List<Map.Entry> evictedEntries = Collections.synchronizedList(new ArrayList<>());

@CacheEntriesEvicted
public void nodeEvicted(CacheEntriesEvictedEvent e){
Expand All @@ -140,11 +156,11 @@ public void nodeEvicted(CacheEntriesEvictedEvent e){
assert key != null;
assert e.getCache() != null;
assert e.getType() == Event.Type.CACHE_ENTRY_EVICTED;
evictedEvents.addAndGet(e.getEntries().size());
e.getEntries().entrySet().stream().forEach(entry -> evictedEntries.add((Map.Entry) entry));
}

public long getEvictedEvents() {
return evictedEvents.get();
public List<Map.Entry> getEvictedEvents() {
return evictedEntries;
}
}
}

0 comments on commit e3dc640

Please sign in to comment.