You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by up...@apache.org on 2016/10/24 18:17:38 UTC
[53/55] [abbrv] incubator-geode git commit: GEODE-706 Fixed race
condition between expiry thread and user thread.
GEODE-706 Fixed race condition between expiry thread and user thread.
ExpirationManager tracks the regionEntry as reference to expiryTask. It
assumes, it is unique for that key. But consistency check mechanism
keeps that regionEntry around and that enforces re-use of that for new
update. That introduces the race condition between expiry thread and
user thread, it endup not scheduling the entry for expiration. As a fix,
now "consistency check mechanism" unschedules the expiry task to avoid
this race condition.
Unmarked flaky from testEntryIdleDestroy test
Project: http://git-wip-us.apache.org/repos/asf/incubator-geode/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-geode/commit/24a72040
Tree: http://git-wip-us.apache.org/repos/asf/incubator-geode/tree/24a72040
Diff: http://git-wip-us.apache.org/repos/asf/incubator-geode/diff/24a72040
Branch: refs/heads/feature/GEODE-1985
Commit: 24a72040cba3afe96da8ef752aa1f07c10dd1fa1
Parents: 6eb0fd3
Author: Hitesh Khamesra <hk...@pivotal.io>
Authored: Fri Oct 21 15:59:16 2016 -0700
Committer: Hitesh Khamesra <hk...@pivotal.io>
Committed: Fri Oct 21 16:09:29 2016 -0700
----------------------------------------------------------------------
.../org/apache/geode/internal/cache/AbstractRegionMap.java | 9 ++++++---
.../org/apache/geode/internal/cache/EntryEventImpl.java | 9 ---------
.../org/apache/geode/internal/cache/EntryExpiryTask.java | 3 +--
.../java/org/apache/geode/internal/cache/LocalRegion.java | 3 ---
.../test/java/org/apache/geode/cache30/RegionTestCase.java | 2 --
5 files changed, 7 insertions(+), 19 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/24a72040/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 b2738ba..de05b0d 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
@@ -1521,9 +1521,11 @@ public abstract class AbstractRegionMap implements RegionMap {
} finally {
if (opCompleted) {
if (re != null) {
- owner.cancelExpiryTask(re, event.getExpiryTask());
- } else if (tombstone != null) {
- owner.cancelExpiryTask(tombstone, event.getExpiryTask());
+ // we only want to cancel if concurrency-check is not enabled
+ // re(regionentry) will be null when concurrency-check is enable and removeTombstone
+ // method
+ // will call cancelExpiryTask on regionEntry
+ owner.cancelExpiryTask(re);
}
}
}
@@ -3801,6 +3803,7 @@ public abstract class AbstractRegionMap implements RegionMap {
try {
re.setValue(_getOwner(), Token.REMOVED_PHASE2);
if (removeTombstone(re)) {
+ _getOwner().cancelExpiryTask(re);
result = true;
incEntryCount(-1);
// Bug 51118: When the method is called by tombstoneGC thread, current 're' is an
http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/24a72040/geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java
----------------------------------------------------------------------
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java b/geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java
index 65ae0f3..02c0422 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java
@@ -69,7 +69,6 @@ public class EntryEventImpl
// PACKAGE FIELDS //
public transient LocalRegion region;
private transient RegionEntry re;
- private transient ExpiryTask expiryTask;
protected KeyInfo keyInfo;
@@ -2820,12 +2819,4 @@ public class EntryEventImpl
public boolean isOldValueOffHeap() {
return isOffHeapReference(this.oldValue);
}
-
- public ExpiryTask getExpiryTask() {
- return expiryTask;
- }
-
- public void setExpiryTask(ExpiryTask expiryTask) {
- this.expiryTask = expiryTask;
- }
}
http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/24a72040/geode-core/src/main/java/org/apache/geode/internal/cache/EntryExpiryTask.java
----------------------------------------------------------------------
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/EntryExpiryTask.java b/geode-core/src/main/java/org/apache/geode/internal/cache/EntryExpiryTask.java
index 603f713..254fc88 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/EntryExpiryTask.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/EntryExpiryTask.java
@@ -111,7 +111,6 @@ public class EntryExpiryTask extends ExpiryTask {
@Released
EntryEventImpl event = EntryEventImpl.create(lr, Operation.EXPIRE_DESTROY, key, null,
createExpireEntryCallback(lr, key), false, lr.getMyId());
- event.setExpiryTask(this);
try {
event.setPendingSecondaryExpireDestroy(isPending);
if (lr.generateEventID()) {
@@ -216,7 +215,7 @@ public class EntryExpiryTask extends ExpiryTask {
// so the next call to addExpiryTaskIfAbsent will
// add a new task instead of doing nothing, which would
// erroneously cancel expiration for this key.
- getLocalRegion().cancelExpiryTask(this.re, null);
+ getLocalRegion().cancelExpiryTask(this.re, this);
getLocalRegion().performExpiryTimeout(this);
}
http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/24a72040/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
----------------------------------------------------------------------
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java b/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
index fce8b5d..360c6a9 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
@@ -8316,7 +8316,6 @@ public class LocalRegion extends AbstractRegion implements LoaderHelperFactory,
if (this.customEntryIdleTimeout != null || this.customEntryTimeToLive != null) {
newTask = createExpiryTask(re);
if (newTask == null) {
- cancelExpiryTask(re); // cancel any old task
return;
}
// to fix bug 44418 see if the new tasks expiration would be earlier than
@@ -8344,7 +8343,6 @@ public class LocalRegion extends AbstractRegion implements LoaderHelperFactory,
if (newTask == null) {
newTask = createExpiryTask(re);
if (newTask == null) {
- cancelExpiryTask(re); // cancel any old task
return;
}
}
@@ -8370,7 +8368,6 @@ public class LocalRegion extends AbstractRegion implements LoaderHelperFactory,
// return;
// }
} else {
- cancelExpiryTask(re);
if (logger.isTraceEnabled()) {
logger.trace("addExpiryTask(key) ignored");
}
http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/24a72040/geode-core/src/test/java/org/apache/geode/cache30/RegionTestCase.java
----------------------------------------------------------------------
diff --git a/geode-core/src/test/java/org/apache/geode/cache30/RegionTestCase.java b/geode-core/src/test/java/org/apache/geode/cache30/RegionTestCase.java
index bee89b6..d87cbd8 100644
--- a/geode-core/src/test/java/org/apache/geode/cache30/RegionTestCase.java
+++ b/geode-core/src/test/java/org/apache/geode/cache30/RegionTestCase.java
@@ -3510,8 +3510,6 @@ public abstract class RegionTestCase extends JUnit4CacheTestCase {
/**
* Tests that an entry in a region that remains idle for a given amount of time is destroyed.
*/
- @Category(FlakyTest.class) // GEODE-706: time sensitive, expiration, waitForDestroy,
- // EXPIRY_MS_PROPERTY, short timeout
@Test
public void testEntryIdleDestroy() throws Exception {