GEODE-185: Fix races in testRegionIdleInvalidate The test code is now careful to wait for the expiration clock to advance. A different assertion will be triggered if the expiration clock goes back in time. Fixed two places in the product expiration code that did not use the expiration clock.
Project: http://git-wip-us.apache.org/repos/asf/incubator-geode/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-geode/commit/b86c3082 Tree: http://git-wip-us.apache.org/repos/asf/incubator-geode/tree/b86c3082 Diff: http://git-wip-us.apache.org/repos/asf/incubator-geode/diff/b86c3082 Branch: refs/heads/feature/GEODE-137 Commit: b86c3082c4dbb85e6929875e666ef63440b02f05 Parents: e7e2426 Author: Darrel Schneider <dschnei...@pivotal.io> Authored: Wed Aug 5 14:17:19 2015 -0700 Committer: Qihong Chen <qc...@pivotal.io> Committed: Thu Aug 6 10:07:48 2015 -0700 ---------------------------------------------------------------------- .../gemfire/internal/cache/ExpiryTask.java | 2 +- .../internal/cache/RegionIdleExpiryTask.java | 3 ++- .../gemstone/gemfire/cache30/RegionTestCase.java | 15 ++++++++++----- .../src/test/java/dunit/DistributedTestCase.java | 19 +++++++++++++++---- 4 files changed, 28 insertions(+), 11 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/b86c3082/gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/ExpiryTask.java ---------------------------------------------------------------------- diff --git a/gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/ExpiryTask.java b/gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/ExpiryTask.java index 95cd3a8..d5dc5ee 100644 --- a/gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/ExpiryTask.java +++ b/gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/ExpiryTask.java @@ -437,7 +437,7 @@ public abstract class ExpiryTask extends SystemTimer.SystemTimerTask { return super.toString() + " for " + getLocalRegion() + ", ttl expiration time: " + expTtl + ", idle expiration time: " + expIdle + - ("[now:" + System.currentTimeMillis() + "]"); + ("[now:" + calculateNow() + "]"); } ////// Abstract methods /////// http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/b86c3082/gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/RegionIdleExpiryTask.java ---------------------------------------------------------------------- diff --git a/gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/RegionIdleExpiryTask.java b/gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/RegionIdleExpiryTask.java index fbcb12c..52975a2 100644 --- a/gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/RegionIdleExpiryTask.java +++ b/gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/RegionIdleExpiryTask.java @@ -39,7 +39,8 @@ class RegionIdleExpiryTask extends RegionExpiryTask { if (!getLocalRegion().EXPIRY_UNITS_MS) { timeout *= 1000; } - return timeout + System.currentTimeMillis(); + // Expiration should always use the DSClock instead of the System clock. + return timeout + getLocalRegion().cacheTimeMillis(); } } // otherwise, expire at timeout plus last accessed time http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/b86c3082/gemfire-core/src/test/java/com/gemstone/gemfire/cache30/RegionTestCase.java ---------------------------------------------------------------------- diff --git a/gemfire-core/src/test/java/com/gemstone/gemfire/cache30/RegionTestCase.java b/gemfire-core/src/test/java/com/gemstone/gemfire/cache30/RegionTestCase.java index 69eeec0..c063a55 100644 --- a/gemfire-core/src/test/java/com/gemstone/gemfire/cache30/RegionTestCase.java +++ b/gemfire-core/src/test/java/com/gemstone/gemfire/cache30/RegionTestCase.java @@ -3853,29 +3853,34 @@ public abstract class RegionTestCase extends CacheTestCase { // expiration time to be extended. // For this phase we don't worry about actually expiring but just // making sure the expiration time gets extended. - region.getAttributesMutator().setRegionIdleTimeout(new ExpirationAttributes(9000/*ms*/, ExpirationAction.INVALIDATE)); + final int EXPIRATION_MS = 9000; + region.getAttributesMutator().setRegionIdleTimeout(new ExpirationAttributes(EXPIRATION_MS, ExpirationAction.INVALIDATE)); LocalRegion lr = (LocalRegion) region; { ExpiryTask expiryTask = lr.getRegionIdleExpiryTask(); region.put(key, value); long createExpiry = expiryTask.getExpirationTime(); - waitForExpiryClockToChange(lr); + long changeTime = waitForExpiryClockToChange(lr, createExpiry-EXPIRATION_MS); region.put(key, "VALUE2"); long putExpiry = expiryTask.getExpirationTime(); + assertTrue("CLOCK went back in time! Expected putBaseExpiry=" + (putExpiry-EXPIRATION_MS) + " to be >= than changeTime=" + changeTime, (putExpiry-EXPIRATION_MS - changeTime) >= 0); assertTrue("expected putExpiry=" + putExpiry + " to be > than createExpiry=" + createExpiry, (putExpiry - createExpiry) > 0); - waitForExpiryClockToChange(lr); + changeTime = waitForExpiryClockToChange(lr, putExpiry-EXPIRATION_MS); region.get(key); long getExpiry = expiryTask.getExpirationTime(); + assertTrue("CLOCK went back in time! Expected getBaseExpiry=" + (getExpiry-EXPIRATION_MS) + " to be >= than changeTime=" + changeTime, (getExpiry-EXPIRATION_MS - changeTime) >= 0); assertTrue("expected getExpiry=" + getExpiry + " to be > than putExpiry=" + putExpiry, (getExpiry - putExpiry) > 0); - waitForExpiryClockToChange(lr); + changeTime = waitForExpiryClockToChange(lr, getExpiry-EXPIRATION_MS); sub.put(key, value); long subPutExpiry = expiryTask.getExpirationTime(); + assertTrue("CLOCK went back in time! Expected subPutBaseExpiry=" + (subPutExpiry-EXPIRATION_MS) + " to be >= than changeTime=" + changeTime, (subPutExpiry-EXPIRATION_MS - changeTime) >= 0); assertTrue("expected subPutExpiry=" + subPutExpiry + " to be > than getExpiry=" + getExpiry, (subPutExpiry - getExpiry) > 0); - waitForExpiryClockToChange(lr); + changeTime = waitForExpiryClockToChange(lr, subPutExpiry-EXPIRATION_MS); sub.get(key); long subGetExpiry = expiryTask.getExpirationTime(); + assertTrue("CLOCK went back in time! Expected subGetBaseExpiry=" + (subGetExpiry-EXPIRATION_MS) + " to be >= than changeTime=" + changeTime, (subGetExpiry-EXPIRATION_MS - changeTime) >= 0); assertTrue("expected subGetExpiry=" + subGetExpiry + " to be > than subPutExpiry=" + subPutExpiry, (subGetExpiry - subPutExpiry) > 0); } } http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/b86c3082/gemfire-core/src/test/java/dunit/DistributedTestCase.java ---------------------------------------------------------------------- diff --git a/gemfire-core/src/test/java/dunit/DistributedTestCase.java b/gemfire-core/src/test/java/dunit/DistributedTestCase.java index 564e7ef..8aa8b6d 100755 --- a/gemfire-core/src/test/java/dunit/DistributedTestCase.java +++ b/gemfire-core/src/test/java/dunit/DistributedTestCase.java @@ -976,13 +976,24 @@ public abstract class DistributedTestCase extends TestCase implements java.io.Se } /** - * Blocks until the clock used for expiration on the given region changes. + * Blocks until the clock used for expiration moves forward. + * @return the last time stamp observed */ - public static final void waitForExpiryClockToChange(LocalRegion lr) { - long startTime = lr.cacheTimeMillis(); + public static final long waitForExpiryClockToChange(LocalRegion lr) { + return waitForExpiryClockToChange(lr, lr.cacheTimeMillis()); + } + /** + * Blocks until the clock used for expiration moves forward. + * @param baseTime the timestamp that the clock must exceed + * @return the last time stamp observed + */ + public static final long waitForExpiryClockToChange(LocalRegion lr, final long baseTime) { + long nowTime; do { Thread.yield(); - } while (startTime == lr.cacheTimeMillis()); + nowTime = lr.cacheTimeMillis(); + } while ((nowTime - baseTime) <= 0L); + return nowTime; } /** pause for specified ms interval