Repository: geode Updated Branches: refs/heads/feature/GEODE-3314 [created] 4643e25fe
GEODE-3314: Initial commit to reproduce DLockToken leak Project: http://git-wip-us.apache.org/repos/asf/geode/repo Commit: http://git-wip-us.apache.org/repos/asf/geode/commit/4643e25f Tree: http://git-wip-us.apache.org/repos/asf/geode/tree/4643e25f Diff: http://git-wip-us.apache.org/repos/asf/geode/diff/4643e25f Branch: refs/heads/feature/GEODE-3314 Commit: 4643e25fe8b4dea7d7160f665529920c3a8789a8 Parents: 09dd45f Author: Udo Kohlmeyer <[email protected]> Authored: Tue Jul 25 16:46:34 2017 -0700 Committer: Udo Kohlmeyer <[email protected]> Committed: Tue Jul 25 16:46:34 2017 -0700 ---------------------------------------------------------------------- .../internal/locks/DLockRequestProcessor.java | 6 - .../internal/locks/DLockService.java | 42 +++--- .../internal/locks/DLockServiceJUnitTest.java | 133 +++++++++++++++++++ 3 files changed, 155 insertions(+), 26 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/geode/blob/4643e25f/geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockRequestProcessor.java ---------------------------------------------------------------------- diff --git a/geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockRequestProcessor.java b/geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockRequestProcessor.java index f0ee31b..3f42adb 100755 --- a/geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockRequestProcessor.java +++ b/geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockRequestProcessor.java @@ -202,18 +202,12 @@ public class DLockRequestProcessor extends ReplyProcessor21 { Assert.assertTrue(lockId > -1, "lockId is < 0: " + this); this.request.lockId = lockId; - // setDoneProcessing(false); - // local grantor... don't use messaging... fake it if (isLockGrantor()) { if (isDebugEnabled_DLS) { logger.trace(LogMarker.DLS, "DLockRequestProcessor processing lock request directly"); } this.request.setSender(this.dm.getDistributionManagerId()); - /* - * if (svc.isDestroyed()) { return false; } - */ - // svc.checkDestroyed(); // calls processor (this) process... this.request.processLocally(this.dm); http://git-wip-us.apache.org/repos/asf/geode/blob/4643e25f/geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockService.java ---------------------------------------------------------------------- diff --git a/geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockService.java b/geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockService.java index c38cdad..0e51bf2 100644 --- a/geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockService.java +++ b/geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockService.java @@ -1388,7 +1388,6 @@ public class DLockService extends DistributedLockService { throw new InterruptedException(); } - boolean abnormalExit = true; boolean safeExit = true; try { // try-block for abnormalExit and safeExit @@ -1438,23 +1437,7 @@ public class DLockService extends DistributedLockService { while (keepTrying) { if (DEBUG_LOCK_REQUEST_LOOP) { loopCount++; - if (loopCount > DEBUG_LOCK_REQUEST_LOOP_COUNT) { - Integer count = Integer.valueOf(DEBUG_LOCK_REQUEST_LOOP_COUNT); - String s = - LocalizedStrings.DLockService_DEBUG_LOCKINTERRUPTIBLY_HAS_GONE_HOT_AND_LOOPED_0_TIMES - .toLocalizedString(count); - - InternalGemFireError e = new InternalGemFireError(s); - logger.error(LogMarker.DLS, - LocalizedMessage.create( - LocalizedStrings.DLockService_DEBUG_LOCKINTERRUPTIBLY_HAS_GONE_HOT_AND_LOOPED_0_TIMES, - count), - e); - throw e; - } - /* - * if (loopCount > 1) { Thread.sleep(1000); } - */ + extracted1(loopCount); } checkDestroyed(); @@ -1552,6 +1535,7 @@ public class DLockService extends DistributedLockService { } // else: non-reentrant or reentrant w/ non-infinite lease if (gotLock) { + token.incUsage(); // if (processor != null) (cannot be null) { // TODO: can be null after restoring above optimization // non-reentrant lock needs to getLeaseExpireTime @@ -1716,7 +1700,6 @@ public class DLockService extends DistributedLockService { logger.trace(LogMarker.DLS, "{}, name: {} - exiting lock() returning {}", this, name, gotLock); } - abnormalExit = false; return gotLock; } // try-block for abnormalExit and safeExit @@ -1735,6 +1718,26 @@ public class DLockService extends DistributedLockService { } } + private void extracted1(int loopCount) { + if (loopCount > DEBUG_LOCK_REQUEST_LOOP_COUNT) { + Integer count = Integer.valueOf(DEBUG_LOCK_REQUEST_LOOP_COUNT); + String s = + LocalizedStrings.DLockService_DEBUG_LOCKINTERRUPTIBLY_HAS_GONE_HOT_AND_LOOPED_0_TIMES + .toLocalizedString(count); + + InternalGemFireError e = new InternalGemFireError(s); + logger.error(LogMarker.DLS, + LocalizedMessage.create( + LocalizedStrings.DLockService_DEBUG_LOCKINTERRUPTIBLY_HAS_GONE_HOT_AND_LOOPED_0_TIMES, + count), + e); + throw e; + } + /* + * if (loopCount > 1) { Thread.sleep(1000); } + */ + } + /** * Allow locking to resume. */ @@ -2659,7 +2662,6 @@ public class DLockService extends DistributedLockService { } getStats().incTokens(1); } - token.incUsage(); } return token; } http://git-wip-us.apache.org/repos/asf/geode/blob/4643e25f/geode-core/src/test/java/org/apache/geode/distributed/internal/locks/DLockServiceJUnitTest.java ---------------------------------------------------------------------- diff --git a/geode-core/src/test/java/org/apache/geode/distributed/internal/locks/DLockServiceJUnitTest.java b/geode-core/src/test/java/org/apache/geode/distributed/internal/locks/DLockServiceJUnitTest.java new file mode 100644 index 0000000..9968f09 --- /dev/null +++ b/geode-core/src/test/java/org/apache/geode/distributed/internal/locks/DLockServiceJUnitTest.java @@ -0,0 +1,133 @@ +package org.apache.geode.distributed.internal.locks; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import org.apache.geode.cache.Cache; +import org.apache.geode.cache.CacheFactory; +import org.apache.geode.cache.ExpirationAction; +import org.apache.geode.cache.ExpirationAttributes; +import org.apache.geode.cache.RegionShortcut; +import org.apache.geode.cache.Scope; +import org.apache.geode.internal.cache.DistributedRegion; +import org.apache.geode.test.junit.categories.UnitTest; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +import java.util.Collection; +import java.util.LinkedList; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import java.util.concurrent.locks.Lock; + +@Category(UnitTest.class) +public class DLockServiceJUnitTest { + + private Cache cache; + private ExecutorService executorService; + private DistributedRegion testRegion; + + @Test + public void basicDLockUsage() throws InterruptedException, ExecutionException, TimeoutException { + Lock lock = testRegion.getDistributedLock("testLockName"); + lock.lockInterruptibly(); + + Future<Boolean> + future = + executorService.submit(() -> lock.tryLock()); + assertFalse("should not be able to get lock from another thread", + future.get(5, TimeUnit.SECONDS)); + + assertTrue("Lock is reentrant", lock.tryLock()); + // now locked twice. + + future = + executorService.submit(() -> lock.tryLock()); + assertFalse("should not be able to get lock from another thread", + future.get(5, TimeUnit.SECONDS)); + + lock.unlock(); + + future = executorService.submit(() -> lock.tryLock()); + assertFalse("should not be able to get lock from another thread", future.get()); + + lock.unlock(); + + future = executorService.submit(() -> { + boolean locked = lock.tryLock(); + if (!locked) { + return false; + } + lock.unlock(); + return true; + }); + assertTrue("Another thread can now take out the lock", future.get(5, TimeUnit.SECONDS)); + + DLockService lockService = (DLockService) testRegion.getLockService(); + Collection<DLockToken> tokens = lockService.getTokens(); + + assertEquals(1, tokens.size()); + + for (DLockToken token : tokens) { + assertEquals(0, token.getUsageCount()); + } + } + + @Before + public void setUp() { + cache = new CacheFactory().create(); + testRegion = (DistributedRegion) cache.createRegionFactory(RegionShortcut.REPLICATE) + .setScope(Scope.GLOBAL) + .setEntryTimeToLive(new ExpirationAttributes(1, ExpirationAction.DESTROY)) + .create("testRegion"); + testRegion.becomeLockGrantor(); + + executorService = Executors.newFixedThreadPool(5); + } + + @After + public void tearDown() { + cache.close(); + executorService.shutdownNow(); + } + + @Test + public void multipleThreadsWithCache() { + LinkedList<Future> futures = new LinkedList<>(); + for (int i = 0; i < 5; i++) { + futures.add(executorService.submit(this::putTestKey)); + } + + futures.stream().forEach(future -> { + try { + future.get(); + } catch (InterruptedException e) { + e.printStackTrace(); + } catch (ExecutionException e) { + e.printStackTrace(); + } + }); + + DLockService lockService = (DLockService) testRegion.getLockService(); + Collection<DLockToken> tokens = lockService.getTokens(); + + assertEquals(1, tokens.size()); + + for (DLockToken token : tokens) { + assertEquals(0, token.getUsageCount()); + } + } + + private void putTestKey() { + for (int i = 0; i < 1000; i++) { + testRegion.put("testKey", "testValue"); + } + } +} \ No newline at end of file
