This is an automated email from the ASF dual-hosted git repository.
jinmeiliao pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/develop by this push:
new bb0a614 GEODE-5262: fix race condition in RemoteGfManagerAgent
between remove… (#2001)
bb0a614 is described below
commit bb0a61489e259c6d2df0ff82a41d559267533ac3
Author: jinmeiliao <[email protected]>
AuthorDate: Wed May 30 14:21:48 2018 -0700
GEODE-5262: fix race condition in RemoteGfManagerAgent between remove…
(#2001)
---
.../admin/remote/RemoteGfManagerAgent.java | 40 ++++++----
.../admin/remote/RemoteGfManagerAgentTest.java | 86 ++++++++++++++++++++++
2 files changed, 111 insertions(+), 15 deletions(-)
diff --git
a/geode-core/src/main/java/org/apache/geode/internal/admin/remote/RemoteGfManagerAgent.java
b/geode-core/src/main/java/org/apache/geode/internal/admin/remote/RemoteGfManagerAgent.java
index 0cc04d2..d91a500 100644
---
a/geode-core/src/main/java/org/apache/geode/internal/admin/remote/RemoteGfManagerAgent.java
+++
b/geode-core/src/main/java/org/apache/geode/internal/admin/remote/RemoteGfManagerAgent.java
@@ -89,7 +89,8 @@ class RemoteGfManagerAgent implements GfManagerAgent {
*
* @since GemFire 4.0
*/
- protected InternalDistributedSystem system;
+ protected volatile InternalDistributedSystem system;
+ private final Object systemLock = new Object();
/** Is this agent connected to the distributed system */
protected volatile boolean connected = false;
@@ -126,7 +127,7 @@ class RemoteGfManagerAgent implements GfManagerAgent {
/**
* The known application VMs. Maps member id to RemoteApplicationVM instances
*/
- private volatile Map membersMap = Collections.EMPTY_MAP;
+ protected volatile Map membersMap = Collections.EMPTY_MAP;
private final Object membersLock = new Object();
// LOG: used to log WARN for AuthenticationFailedException
@@ -403,12 +404,13 @@ class RemoteGfManagerAgent implements GfManagerAgent {
// ignore a forced disconnect and finish clean-up
}
- if (system != null && ClusterDistributionManager.isDedicatedAdminVM()
- && system.isConnected()) {
- system.disconnect();
+ synchronized (systemLock) {
+ if (system != null && ClusterDistributionManager.isDedicatedAdminVM()
+ && system.isConnected()) {
+ system.disconnect();
+ }
+ this.system = null;
}
-
- this.system = null;
this.connected = false;
}
@@ -705,7 +707,13 @@ class RemoteGfManagerAgent implements GfManagerAgent {
if (future != null) {
future.cancel(true);
for (;;) {
- this.system.getCancelCriterion().checkCancelInProgress(null);
+ synchronized (systemLock) {
+ if (system == null) {
+ return null;
+ }
+ this.system.getCancelCriterion().checkCancelInProgress(null);
+ }
+
boolean interrupted = Thread.interrupted();
try {
return (RemoteApplicationVM) future.get();
@@ -793,17 +801,19 @@ class RemoteGfManagerAgent implements GfManagerAgent {
if (!isListening()) {
return;
}
-
- if (system != null) {
- system.disconnect();
- system = null;
- }
-
Properties props = this.transport.toDSProperties();
if (this.displayName != null && this.displayName.length() > 0) {
props.setProperty("name", this.displayName);
}
- this.system = (InternalDistributedSystem)
InternalDistributedSystem.connectForAdmin(props);
+
+ synchronized (systemLock) {
+ if (system != null) {
+ system.disconnect();
+ system = null;
+ }
+ this.system = (InternalDistributedSystem)
InternalDistributedSystem.connectForAdmin(props);
+ }
+
DistributionManager dm = system.getDistributionManager();
if (dm instanceof ClusterDistributionManager) {
((ClusterDistributionManager) dm).setAgent(this);
diff --git
a/geode-core/src/test/java/org/apache/geode/internal/admin/remote/RemoteGfManagerAgentTest.java
b/geode-core/src/test/java/org/apache/geode/internal/admin/remote/RemoteGfManagerAgentTest.java
new file mode 100644
index 0000000..401d5b2
--- /dev/null
+++
b/geode-core/src/test/java/org/apache/geode/internal/admin/remote/RemoteGfManagerAgentTest.java
@@ -0,0 +1,86 @@
+/*
+ * 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.internal.admin.remote;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.when;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import
org.apache.geode.distributed.internal.membership.InternalDistributedMember;
+import org.apache.geode.distributed.internal.membership.gms.Services;
+import org.apache.geode.internal.admin.GfManagerAgentConfig;
+import org.apache.geode.internal.logging.InternalLogWriter;
+import org.apache.geode.test.junit.categories.UnitTest;
+
+
+@Category(UnitTest.class)
+public class RemoteGfManagerAgentTest {
+
+ private RemoteGfManagerAgent mockConnectedAgent(GfManagerAgentConfig config)
{
+ RemoteGfManagerAgent agent = spy(new RemoteGfManagerAgent(config));
+ InternalDistributedSystem system = mock(InternalDistributedSystem.class);
+ when(system.isConnected()).thenReturn(true);
+ when(system.getCancelCriterion()).thenReturn(mock(Services.Stopper.class));
+ agent.system = system;
+ agent.connected = true;
+ return agent;
+ }
+
+ @Test
+ public void removeAgentAndDisconnectWouldNotThrowNPE()
+ throws InterruptedException, ExecutionException {
+ InternalDistributedMember member;
+ member = mock(InternalDistributedMember.class);
+ Future future = mock(Future.class);
+
+ GfManagerAgentConfig config = mock(GfManagerAgentConfig.class);
+ when(config.getTransport()).thenReturn(mock(RemoteTransportConfig.class));
+ when(config.getLogWriter()).thenReturn(mock(InternalLogWriter.class));
+
+ int count = 10;
+
+ for (int i = 0; i < count; i++) {
+ RemoteGfManagerAgent agent = mockConnectedAgent(config);
+ Map membersMap = new HashMap();
+ membersMap.put(member, future);
+ agent.membersMap = membersMap;
+ ExecutorService es = Executors.newFixedThreadPool(2);
+
+ // removeMember accesses the InternalDistributedSystem
+ Future<?> future1 = es.submit(() -> {
+ agent.removeMember(member);
+ });
+
+ // disconnect sets the InternalDistributedSystem to null
+ Future<?> future2 = es.submit(() -> {
+ agent.disconnect();
+ });
+
+ future1.get();
+ future2.get();
+ }
+ }
+}
--
To stop receiving notification emails like this one, please contact
[email protected].
