You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by js...@apache.org on 2017/08/30 16:45:39 UTC
geode git commit: GEODE-2859: Fix race condition in
ShowDeadlockDUnitTest
Repository: geode
Updated Branches:
refs/heads/develop fb9a405f7 -> ca0dca51e
GEODE-2859: Fix race condition in ShowDeadlockDUnitTest
Project: http://git-wip-us.apache.org/repos/asf/geode/repo
Commit: http://git-wip-us.apache.org/repos/asf/geode/commit/ca0dca51
Tree: http://git-wip-us.apache.org/repos/asf/geode/tree/ca0dca51
Diff: http://git-wip-us.apache.org/repos/asf/geode/diff/ca0dca51
Branch: refs/heads/develop
Commit: ca0dca51ebd9694cf6a34548252ed3c720b59253
Parents: fb9a405
Author: Jared Stewart <js...@pivotal.io>
Authored: Tue Aug 29 12:02:47 2017 -0700
Committer: Jared Stewart <js...@pivotal.io>
Committed: Wed Aug 30 09:44:59 2017 -0700
----------------------------------------------------------------------
.../cli/commands/ShowDeadlockDUnitTest.java | 205 +++++++------------
.../dunit/rules/GfshShellConnectionRule.java | 9 +-
geode-junit/build.gradle | 1 +
.../concurrent/FileBasedCountDownLatch.java | 87 ++++++++
.../concurrent/FileBasedCountDownLatchTest.java | 37 ++++
5 files changed, 202 insertions(+), 137 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/geode/blob/ca0dca51/geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ShowDeadlockDUnitTest.java
----------------------------------------------------------------------
diff --git a/geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ShowDeadlockDUnitTest.java b/geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ShowDeadlockDUnitTest.java
index 4df0b96..51560db 100755
--- a/geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ShowDeadlockDUnitTest.java
+++ b/geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ShowDeadlockDUnitTest.java
@@ -14,203 +14,138 @@
*/
package org.apache.geode.management.internal.cli.commands;
-import static org.apache.geode.test.dunit.Assert.assertEquals;
-import static org.apache.geode.test.dunit.Assert.assertTrue;
-import static org.apache.geode.test.dunit.Invoke.invokeInEveryVM;
+import static org.assertj.core.api.Assertions.assertThat;
import java.io.File;
-import java.io.IOException;
-import java.util.Collections;
-import java.util.HashSet;
-import java.util.Properties;
-import java.util.Set;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
-import org.apache.commons.io.FileUtils;
import org.awaitility.Awaitility;
import org.junit.After;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.experimental.categories.Category;
+import org.junit.rules.TemporaryFolder;
import org.apache.geode.cache.execute.Function;
import org.apache.geode.cache.execute.FunctionContext;
import org.apache.geode.cache.execute.FunctionService;
import org.apache.geode.cache.execute.ResultCollector;
-import org.apache.geode.cache30.CacheTestCase;
-import org.apache.geode.distributed.internal.deadlock.GemFireDeadlockDetector;
import org.apache.geode.distributed.internal.deadlock.GemFireDeadlockDetectorDUnitTest;
import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
-import org.apache.geode.management.cli.CommandStatement;
-import org.apache.geode.management.cli.Result;
-import org.apache.geode.management.cli.Result.Status;
import org.apache.geode.management.internal.cli.i18n.CliStrings;
-import org.apache.geode.management.internal.cli.remote.CommandProcessor;
-import org.apache.geode.management.internal.cli.util.CommandStringBuilder;
-import org.apache.geode.test.dunit.Host;
-import org.apache.geode.test.dunit.SerializableCallable;
-import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.concurrent.FileBasedCountDownLatch;
+import org.apache.geode.test.dunit.rules.GfshShellConnectionRule;
+import org.apache.geode.test.dunit.rules.LocatorServerStartupRule;
+import org.apache.geode.test.dunit.rules.MemberVM;
import org.apache.geode.test.junit.categories.DistributedTest;
-import org.apache.geode.test.junit.rules.serializable.SerializableTemporaryFolder;
/**
- *
+ * Distributed tests for show deadlock command in {@link ShowDeadlockCommand}.
+ *
* @see GemFireDeadlockDetectorDUnitTest
*/
@Category(DistributedTest.class)
-public class ShowDeadlockDUnitTest extends CacheTestCase {
+public class ShowDeadlockDUnitTest {
+ private static Thread stuckThread = null;
+ private static final Lock LOCK = new ReentrantLock();
- private static final Set<Thread> stuckThreads =
- Collections.synchronizedSet(new HashSet<Thread>());
+ private MemberVM server1;
+ private MemberVM server2;
- private static final Lock lock = new ReentrantLock();
+ private File outputFile;
+ private String showDeadlockCommand;
- private transient VM vm0;
- private transient VM vm1;
+ @Rule
+ public LocatorServerStartupRule lsRule = new LocatorServerStartupRule();
- private transient InternalDistributedMember member0;
- private transient InternalDistributedMember member1;
+ @Rule
+ public TemporaryFolder temporaryFolder = new TemporaryFolder();
@Rule
- public SerializableTemporaryFolder temporaryFolder = new SerializableTemporaryFolder();
+ public GfshShellConnectionRule gfsh = new GfshShellConnectionRule();
@Before
- public void setup() {
- Host host = Host.getHost(0);
- vm0 = host.getVM(0);
- vm1 = host.getVM(1);
-
- // Make sure a deadlock from a previous test is cleared.
- disconnectAllFromDS();
+ public void setup() throws Exception {
+ outputFile = new File(temporaryFolder.getRoot(), "dependency.txt").getAbsoluteFile();
+ showDeadlockCommand = "show dead-locks --file=" + outputFile.getAbsolutePath();
+ outputFile.delete();
- member0 = createCache(vm0);
- member1 = createCache(vm1);
+ MemberVM locator = lsRule.startLocatorVM(0);
+ server1 = lsRule.startServerVM(1, locator.getPort());
+ server2 = lsRule.startServerVM(2, locator.getPort());
- createCache(new Properties());
+ gfsh.connect(locator);
}
@After
- public void teardown() {
- disconnectAllFromDS();
- }
-
- @Override
- public final void preTearDownCacheTestCase() throws Exception {
- invokeInEveryVM(() -> stuckThreads.forEach(Thread::interrupt));
+ public final void after() throws Exception {
+ server1.invoke(() -> stuckThread.interrupt());
+ server2.invoke(() -> stuckThread.interrupt());
}
@Test
public void testNoDeadlock() throws Exception {
- GemFireDeadlockDetector detect = new GemFireDeadlockDetector();
- assertEquals(null, detect.find().findCycle());
-
- File outputFile = new File(temporaryFolder.getRoot(), "dependency.txt");
+ gfsh.executeAndVerifyCommand(showDeadlockCommand);
+ String commandOutput = gfsh.getGfshOutput();
- String showDeadlockCommand = new CommandStringBuilder(CliStrings.SHOW_DEADLOCK)
- .addOption(CliStrings.SHOW_DEADLOCK__DEPENDENCIES__FILE, outputFile.getName()).toString();
-
- Result result = new CommandProcessor()
- .createCommandStatement(showDeadlockCommand, Collections.emptyMap()).process();
- String commandOutput = getResultAsString(result);
-
- assertEquals(true, result.hasIncomingFiles());
- assertEquals(true, result.getStatus().equals(Status.OK));
- assertEquals(true, commandOutput.startsWith(CliStrings.SHOW_DEADLOCK__NO__DEADLOCK));
- result.saveIncomingFiles(temporaryFolder.getRoot().getAbsolutePath());
- assertTrue(outputFile.exists());
+ assertThat(commandOutput).startsWith(CliStrings.SHOW_DEADLOCK__NO__DEADLOCK);
+ assertThat(outputFile).exists();
}
@Test
public void testDistributedDeadlockWithFunction() throws Exception {
- // Have two threads lock locks on different members in different orders.
- // This thread locks the lock member0 first, then member1.
- lockTheLocks(vm0, member1);
- // This thread locks the lock member1 first, then member0.
- lockTheLocks(vm1, member0);
-
- File outputFile = new File(temporaryFolder.getRoot(), "dependency.txt");
-
- String showDeadlockCommand = new CommandStringBuilder(CliStrings.SHOW_DEADLOCK)
- .addOption(CliStrings.SHOW_DEADLOCK__DEPENDENCIES__FILE, outputFile.getName()).toString();
- CommandStatement showDeadlocksCommand =
- new CommandProcessor().createCommandStatement(showDeadlockCommand, Collections.emptyMap());
-
- Awaitility.await().atMost(1, TimeUnit.MINUTES).until(() -> {
- FileUtils.deleteQuietly(outputFile);
- Result result = showDeadlocksCommand.process();
- try {
- result.saveIncomingFiles(temporaryFolder.getRoot().getAbsolutePath());
- } catch (IOException e) {
- throw new RuntimeException(e);
- }
-
- String commandOutput = getResultAsString(result);
- assertEquals(true, commandOutput.startsWith(CliStrings.SHOW_DEADLOCK__DEADLOCK__DETECTED));
- assertEquals(true, result.getStatus().equals(Status.OK));
- assertTrue(outputFile.exists());
+ FileBasedCountDownLatch countDownLatch = new FileBasedCountDownLatch(2);
+
+ // This thread locks the lock in server1 first, then server2.
+ lockTheLocks(server1, server2, countDownLatch);
+ // This thread locks the lock server2 first, then server1.
+ lockTheLocks(server2, server1, countDownLatch);
+
+ Awaitility.await().atMost(5, TimeUnit.MINUTES).pollDelay(5, TimeUnit.SECONDS).until(() -> {
+ gfsh.executeAndVerifyCommand(showDeadlockCommand);
+ String commandOutput = gfsh.getGfshOutput();
+ assertThat(commandOutput).startsWith(CliStrings.SHOW_DEADLOCK__DEADLOCK__DETECTED);
+ assertThat(outputFile).exists();
});
}
- private void createCache(Properties props) {
- getSystem(props);
- getCache();
- }
-
- private void lockTheLocks(VM vm0, final InternalDistributedMember member) {
- vm0.invokeAsync(() -> {
- lock.lock();
-
- ResultCollector collector = FunctionService.onMember(member).execute(new TestFunction());
- // wait the function to lock the lock on member.
- collector.getResult();
- lock.unlock();
+ private void lockTheLocks(MemberVM thisVM, final MemberVM thatVM,
+ FileBasedCountDownLatch countDownLatch) {
+ thisVM.invokeAsync(() -> {
+ LOCK.lock();
+ countDownLatch.countDown();
+ countDownLatch.await();
+ // At this point each VM will hold its own lock.
+ lockRemoteVM(thatVM);
+ LOCK.unlock();
});
}
- private InternalDistributedMember createCache(VM vm) {
- return (InternalDistributedMember) vm.invoke(new SerializableCallable<Object>() {
- @Override
- public Object call() {
- getCache();
- return getSystem().getDistributedMember();
- }
- });
- }
+ private static void lockRemoteVM(MemberVM vmToLock) {
+ InternalDistributedMember thatInternalMember = getInternalDistributedMember(vmToLock);
- private String getResultAsString(Result result) {
- StringBuilder sb = new StringBuilder();
- while (result.hasNextLine()) {
- sb.append(result.nextLine());
- }
-
- return sb.toString();
+ ResultCollector collector =
+ FunctionService.onMember(thatInternalMember).execute(new LockFunction());
+ collector.getResult();
}
- private static class TestFunction implements Function<Object> {
- private static final int LOCK_WAIT_TIME = 1000;
-
- @Override
- public boolean hasResult() {
- return true;
- }
+ private static InternalDistributedMember getInternalDistributedMember(MemberVM memberVM) {
+ return memberVM.getVM().invoke(() -> LocatorServerStartupRule.serverStarter.getCache()
+ .getInternalDistributedSystem().getDistributedMember());
+ }
+ private static class LockFunction implements Function<Object> {
@Override
public void execute(FunctionContext<Object> context) {
+ stuckThread = Thread.currentThread();
try {
- stuckThreads.add(Thread.currentThread());
- lock.tryLock(LOCK_WAIT_TIME, TimeUnit.SECONDS);
- } catch (InterruptedException ignored) {
- // ignored
+ LOCK.tryLock(5, TimeUnit.MINUTES);
+ } catch (InterruptedException e) {
+ context.getResultSender().lastResult(null);
}
- context.getResultSender().lastResult(null);
- }
-
- @Override
- public boolean isHA() {
- return false;
}
}
}
http://git-wip-us.apache.org/repos/asf/geode/blob/ca0dca51/geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java
----------------------------------------------------------------------
diff --git a/geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java b/geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java
index e7f17ef..a9ce889 100644
--- a/geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java
+++ b/geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java
@@ -223,8 +223,13 @@ public class GfshShellConnectionRule extends DescribedExternalResource {
}
- public CommandResult executeAndVerifyCommand(String command) throws Exception {
- CommandResult result = executeCommand(command);
+ public CommandResult executeAndVerifyCommand(String command) {
+ CommandResult result = null;
+ try {
+ result = executeCommand(command);
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
assertThat(result.getStatus())
.describedAs("Failure in command: " + command + "\n Result " + result)
.isEqualTo(Result.Status.OK);
http://git-wip-us.apache.org/repos/asf/geode/blob/ca0dca51/geode-junit/build.gradle
----------------------------------------------------------------------
diff --git a/geode-junit/build.gradle b/geode-junit/build.gradle
index 7c533ad..ccfbb24 100755
--- a/geode-junit/build.gradle
+++ b/geode-junit/build.gradle
@@ -23,6 +23,7 @@ dependencies {
compile 'commons-io:commons-io:' + project.'commons-io.version'
compile 'commons-lang:commons-lang:' + project.'commons-lang.version'
compile 'com.google.guava:guava:' + project.'guava.version'
+ compile 'org.awaitility:awaitility:' + project.'awaitility.version'
compile('junit:junit:' + project.'junit.version') {
http://git-wip-us.apache.org/repos/asf/geode/blob/ca0dca51/geode-junit/src/main/java/org/apache/geode/test/concurrent/FileBasedCountDownLatch.java
----------------------------------------------------------------------
diff --git a/geode-junit/src/main/java/org/apache/geode/test/concurrent/FileBasedCountDownLatch.java b/geode-junit/src/main/java/org/apache/geode/test/concurrent/FileBasedCountDownLatch.java
new file mode 100644
index 0000000..43fe260
--- /dev/null
+++ b/geode-junit/src/main/java/org/apache/geode/test/concurrent/FileBasedCountDownLatch.java
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.test.concurrent;
+
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.is;
+
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.Serializable;
+import java.util.concurrent.TimeUnit;
+
+import com.google.common.base.Charsets;
+import org.apache.commons.io.FileUtils;
+import org.awaitility.Awaitility;
+
+/**
+ * This is an implementation of CountDownLatch that can be serialized and used across multiple DUnit
+ * VMs. File locks are used to synchronize between the separate VMs. For an example usage, see
+ * ShowDeadlockDUnitTest.
+ */
+public class FileBasedCountDownLatch implements Serializable {
+ private final File lockFile;
+ private final File dataFile;
+
+ public FileBasedCountDownLatch(int count) throws IOException {
+ lockFile = File.createTempFile("CountDownLatchLock", ".txt");
+ dataFile = File.createTempFile("CountDownLatchData", ".txt");
+
+ try (FileOutputStream out = new FileOutputStream(lockFile)) {
+ java.nio.channels.FileLock lock = out.getChannel().lock();
+ try {
+ FileUtils.writeStringToFile(dataFile, String.valueOf(count), Charsets.UTF_8);
+ } finally {
+ lock.release();
+ }
+ }
+
+ lockFile.deleteOnExit();
+ }
+
+ public void countDown() throws IOException {
+ try (FileOutputStream out = new FileOutputStream(lockFile)) {
+ java.nio.channels.FileLock lock = out.getChannel().lock();
+
+ try {
+ String fileContents = FileUtils.readFileToString(dataFile, Charsets.UTF_8);
+ int currentValue = Integer.valueOf(fileContents);
+
+ int newValue = currentValue - 1;
+ FileUtils.writeStringToFile(dataFile, String.valueOf(newValue), Charsets.UTF_8);
+
+ } finally {
+ lock.release();
+ }
+ }
+ }
+
+ public void await() throws IOException {
+ Awaitility.await().atMost(10, TimeUnit.MINUTES).until(this::currentValue, is(equalTo(0)));
+ }
+
+ protected int currentValue() throws IOException {
+ try (FileOutputStream out = new FileOutputStream(lockFile)) {
+ java.nio.channels.FileLock lock = out.getChannel().lock();
+ try {
+ String fileContents = FileUtils.readFileToString(dataFile, Charsets.UTF_8);
+ return Integer.valueOf(fileContents);
+ } finally {
+ lock.release();
+ }
+ }
+ }
+}
http://git-wip-us.apache.org/repos/asf/geode/blob/ca0dca51/geode-junit/src/test/java/org/apache/geode/test/concurrent/FileBasedCountDownLatchTest.java
----------------------------------------------------------------------
diff --git a/geode-junit/src/test/java/org/apache/geode/test/concurrent/FileBasedCountDownLatchTest.java b/geode-junit/src/test/java/org/apache/geode/test/concurrent/FileBasedCountDownLatchTest.java
new file mode 100644
index 0000000..a4da5fb
--- /dev/null
+++ b/geode-junit/src/test/java/org/apache/geode/test/concurrent/FileBasedCountDownLatchTest.java
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.test.concurrent;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import org.apache.geode.test.junit.categories.IntegrationTest;
+
+@Category(IntegrationTest.class)
+public class FileBasedCountDownLatchTest {
+ @Test
+ public void singleThreadedBehaviorIsCorrect() throws Exception {
+ FileBasedCountDownLatch fileBasedCountDownLatch = new FileBasedCountDownLatch(2);
+ assertThat(fileBasedCountDownLatch.currentValue()).isEqualTo(2);
+
+ fileBasedCountDownLatch.countDown();
+ assertThat(fileBasedCountDownLatch.currentValue()).isEqualTo(1);
+
+ fileBasedCountDownLatch.countDown();
+ assertThat(fileBasedCountDownLatch.currentValue()).isEqualTo(0);
+ }
+}