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/GEM-983 Commit: 24a72040cba3afe96da8ef752aa1f07c10dd1fa1 Parents: 6eb0fd3 Author: Hitesh Khamesra <hkhame...@pivotal.io> Authored: Fri Oct 21 15:59:16 2016 -0700 Committer: Hitesh Khamesra <hkhame...@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 {