You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by ds...@apache.org on 2016/09/26 21:47:37 UTC
incubator-geode git commit: GEODE-1885: fix infinite loop
Repository: incubator-geode
Updated Branches:
refs/heads/develop 6555c3120 -> 55a65840a
GEODE-1885: fix infinite loop
The previous fix for GEODE-1885 introduced a hang on off-heap regions.
If a concurrent close/destroy of the region happens while other threads
are modifying it then the thread doing the modification can get stuck
in a hot loop that never terminates.
The hot loop is in AbstractRegionMap when it tests the existing
region entry it finds to see if it can be modified.
If the region entry has a value that says it is removed
then the operation spins around and tries again.
It expects the thread that marked it as being removed
to also remove it from the map.
The previous fix for GEODE-1885 can cause a remove to not happen.
So this fix does two things:
1. On retry remove the existing removed region entry from the map.
2. putEntryIfAbsent now only releases the current entry if it has an off-heap reference.
This prevents an infinite loop that was caused by the current thread who just added
a new entry with REMOVE_PHASE1 from releasing it (changing it to REMOVE_PHASE2)
because it sees that the region is closed/destroyed.
Project: http://git-wip-us.apache.org/repos/asf/incubator-geode/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-geode/commit/55a65840
Tree: http://git-wip-us.apache.org/repos/asf/incubator-geode/tree/55a65840
Diff: http://git-wip-us.apache.org/repos/asf/incubator-geode/diff/55a65840
Branch: refs/heads/develop
Commit: 55a65840a4e4d427acaed1182aca869bf92ecae6
Parents: 6555c31
Author: Darrel Schneider <ds...@pivotal.io>
Authored: Fri Sep 23 10:53:35 2016 -0700
Committer: Darrel Schneider <ds...@pivotal.io>
Committed: Mon Sep 26 14:43:12 2016 -0700
----------------------------------------------------------------------
.../geode/internal/cache/AbstractRegionMap.java | 59 ++++++++++----------
1 file changed, 30 insertions(+), 29 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/55a65840/geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java
----------------------------------------------------------------------
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java b/geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java
index 33e98b6..5861e9a 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java
@@ -48,6 +48,7 @@ import org.apache.geode.internal.logging.log4j.LogMarker;
import org.apache.geode.internal.offheap.OffHeapHelper;
import org.apache.geode.internal.offheap.OffHeapRegionEntryHelper;
import org.apache.geode.internal.offheap.ReferenceCountHelper;
+import org.apache.geode.internal.offheap.StoredObject;
import org.apache.geode.internal.offheap.annotations.Released;
import org.apache.geode.internal.offheap.annotations.Retained;
import org.apache.geode.internal.offheap.annotations.Unretained;
@@ -246,15 +247,19 @@ public abstract class AbstractRegionMap implements RegionMap {
public final RegionEntry putEntryIfAbsent(Object key, RegionEntry re) {
- RegionEntry value = (RegionEntry)_getMap().putIfAbsent(key, re);
- if (value == null && (re instanceof OffHeapRegionEntry)
+ RegionEntry oldRe = (RegionEntry)_getMap().putIfAbsent(key, re);
+ if (oldRe == null && (re instanceof OffHeapRegionEntry)
&& _isOwnerALocalRegion() && _getOwner().isThisRegionBeingClosedOrDestroyed()) {
// prevent orphan during concurrent destroy (#48068)
- if (_getMap().remove(key, re)) {
- ((OffHeapRegionEntry)re).release();
+ Object v = re._getValue();
+ if (v != Token.REMOVED_PHASE1 && v != Token.REMOVED_PHASE2
+ && v instanceof StoredObject && ((StoredObject)v).hasRefCount()) {
+ if (_getMap().remove(key, re)) {
+ ((OffHeapRegionEntry)re).release();
+ }
}
}
- return value;
+ return oldRe;
}
@Override
@@ -640,12 +645,11 @@ public abstract class AbstractRegionMap implements RegionMap {
while (oldRe != null) {
synchronized (oldRe) {
if (oldRe.isRemoved() && !oldRe.isTombstone()) {
- oldRe = putEntryIfAbsent(key, newRe);
- if (oldRe != null) {
- if (_isOwnerALocalRegion()) {
- _getOwner().getCachePerfStats().incRetries();
- }
+ if (_isOwnerALocalRegion()) {
+ _getOwner().getCachePerfStats().incRetries();
}
+ _getMap().remove(key, oldRe);
+ oldRe = putEntryIfAbsent(key, newRe);
}
/*
* Entry already exists which should be impossible.
@@ -844,10 +848,9 @@ public abstract class AbstractRegionMap implements RegionMap {
while (!done && oldRe != null) {
synchronized (oldRe) {
if (oldRe.isRemovedPhase2()) {
+ owner.getCachePerfStats().incRetries();
+ _getMap().remove(key, oldRe);
oldRe = putEntryIfAbsent(key, newRe);
- if (oldRe != null) {
- owner.getCachePerfStats().incRetries();
- }
}
else {
boolean acceptedVersionTag = false;
@@ -1104,10 +1107,9 @@ public abstract class AbstractRegionMap implements RegionMap {
while (!opCompleted && oldRe != null) {
synchronized (oldRe) {
if (oldRe.isRemovedPhase2()) {
+ owner.getCachePerfStats().incRetries();
+ _getMap().remove(event.getKey(), oldRe);
oldRe = putEntryIfAbsent(event.getKey(), newRe);
- if (oldRe != null) {
- owner.getCachePerfStats().incRetries();
- }
} else {
event.setRegionEntry(oldRe);
@@ -1356,6 +1358,8 @@ public abstract class AbstractRegionMap implements RegionMap {
&& (event.isOriginRemote() || event.getContext() != null || removeRecoveredEntry));
if (!re.isRemoved() || createTombstoneForConflictChecks) {
if (re.isRemovedPhase2()) {
+ _getMap().remove(event.getKey(), re);
+ owner.getCachePerfStats().incRetries();
retry = true;
continue;
}
@@ -1650,10 +1654,9 @@ public abstract class AbstractRegionMap implements RegionMap {
while (!opCompleted && oldRe != null) {
synchronized (oldRe) {
if (oldRe.isRemovedPhase2()) {
+ owner.getCachePerfStats().incRetries();
+ _getMap().remove(key, oldRe);
oldRe = putEntryIfAbsent(key, newRe);
- if (oldRe != null) {
- owner.getCachePerfStats().incRetries();
- }
}
else {
try {
@@ -1874,10 +1877,9 @@ public abstract class AbstractRegionMap implements RegionMap {
// that is destroying the RE will see the invalidation and not
// proceed to phase 2 of removal.
if (oldRe.isRemovedPhase2()) {
+ owner.getCachePerfStats().incRetries();
+ _getMap().remove(event.getKey(), oldRe);
oldRe = putEntryIfAbsent(event.getKey(), newRe);
- if (oldRe != null) {
- owner.getCachePerfStats().incRetries();
- }
} else {
opCompleted = true;
event.setRegionEntry(oldRe);
@@ -2335,10 +2337,9 @@ public abstract class AbstractRegionMap implements RegionMap {
while (!opCompleted && oldRe != null) {
synchronized (oldRe) {
if (oldRe.isRemovedPhase2()) {
+ owner.getCachePerfStats().incRetries();
+ _getMap().remove(key, oldRe);
oldRe = putEntryIfAbsent(key, newRe);
- if (oldRe != null) {
- owner.getCachePerfStats().incRetries();
- }
}
else {
opCompleted = true;
@@ -2711,8 +2712,9 @@ public abstract class AbstractRegionMap implements RegionMap {
// from the map. otherwise we can append an event to it
// and change its state
if (re.isRemovedPhase2()) {
- re = getOrCreateRegionEntry(owner, event, Token.REMOVED_PHASE1, null, onlyExisting, false);
_getOwner().getCachePerfStats().incRetries();
+ _getMap().remove(event.getKey(), re);
+ re = getOrCreateRegionEntry(owner, event, Token.REMOVED_PHASE1, null, onlyExisting, false);
if (re == null) {
// this will happen when onlyExisting is true
return null;
@@ -3192,10 +3194,9 @@ public abstract class AbstractRegionMap implements RegionMap {
while (!opCompleted && oldRe != null) {
synchronized (oldRe) {
if (oldRe.isRemovedPhase2()) {
+ owner.getCachePerfStats().incRetries();
+ _getMap().remove(key, oldRe);
oldRe = putEntryIfAbsent(key, newRe);
- if (oldRe != null) {
- owner.getCachePerfStats().incRetries();
- }
}
else {
opCompleted = true;