GEODE-10286: handle CancelException in PersistenceAdvisor.close (apac…

…he#7677)

geode-core/src/main/java/org/apache/geode/internal/cache/DistributedRegion.java

@@ -2604,6 +2604,7 @@ protected void distributedRegionCleanup(RegionEventImpl event) {
             ex);
       }
     }
+
     waitForCurrentOperations();
   }
 
@@ -2615,9 +2616,9 @@ private void waitForCurrentOperations() {
     if (!cache.forcedDisconnect() && flushOnClose
         && getDistributionManager().getDistribution() != null
         && getDistributionManager().getDistribution().isConnected()) {
-      getDistributionAdvisor().forceNewMembershipVersion();
+      distAdvisor.forceNewMembershipVersion();
       try {
-        getDistributionAdvisor().waitForCurrentOperations();
+        distAdvisor.waitForCurrentOperations();
       } catch (Exception e) {
         // log this but try to close the region so that listeners are invoked
         logger.warn(String.format("%s: error closing region %s", this, getFullPath()), e);

geode-core/src/main/java/org/apache/geode/internal/cache/persistence/PersistenceAdvisorImpl.java

@@ -30,6 +30,7 @@
 import org.apache.logging.log4j.Logger;
 import org.jetbrains.annotations.TestOnly;
 
+import org.apache.geode.CancelException;
 import org.apache.geode.annotations.Immutable;
 import org.apache.geode.annotations.VisibleForTesting;
 import org.apache.geode.annotations.internal.MutableForTesting;
@@ -1161,11 +1162,19 @@ public void checkInterruptedByShutdownAll() {
 
   @Override
   public void close() {
-    isClosed = true;
-    persistentMemberManager.removeRevocationListener(profileChangeListener);
-    cacheDistributionAdvisor.removeProfileChangeListener(profileChangeListener);
-    persistentStateListeners = Collections.emptySet();
-    releaseTieLock();
+    try {
+      synchronized (this) {
+        isClosed = true;
+        persistentMemberManager.removeRevocationListener(profileChangeListener);
+        cacheDistributionAdvisor.removeProfileChangeListener(profileChangeListener);
+        persistentStateListeners = Collections.emptySet();
+        releaseTieLock();
+      }
+    } catch (CancelException e) {
+      logger.debug("persistence advisor close abridged due to shutdown", e);
+    } catch (Exception ex) {
+      logger.warn("persistence advisor close has failed.", ex);
+    }
   }
 
   /**

geode-core/src/test/java/org/apache/geode/internal/cache/DistributedRegionTest.java

@@ -30,8 +30,8 @@
 
 import java.util.Collections;
 
-import org.junit.Before;
-import org.junit.Test;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
 import org.mockito.InOrder;
 
 import org.apache.geode.cache.DataPolicy;
@@ -53,8 +53,9 @@ public class DistributedRegionTest {
   private InternalDistributedMember member;
   private EntryEventImpl event;
   private EventTracker eventTracker;
+  private DistributedRegion distributedRegion;
 
-  @Before
+  @BeforeEach
   @SuppressWarnings("unchecked")
   public void setup() {
     vector = mock(RegionVersionVector.class);
@@ -63,18 +64,18 @@ public void setup() {
     member = mock(InternalDistributedMember.class);
     event = mock(EntryEventImpl.class);
     eventTracker = mock(EventTracker.class);
+    distributedRegion = mock(DistributedRegion.class);
   }
 
   @Test
   public void shouldBeMockable() throws Exception {
-    DistributedRegion mockDistributedRegion = mock(DistributedRegion.class);
     EntryEventImpl mockEntryEventImpl = mock(EntryEventImpl.class);
     Object returnValue = new Object();
 
-    when(mockDistributedRegion.validatedDestroy(any(), eq(mockEntryEventImpl)))
+    when(distributedRegion.validatedDestroy(any(), eq(mockEntryEventImpl)))
         .thenReturn(returnValue);
 
-    assertThat(mockDistributedRegion.validatedDestroy(new Object(), mockEntryEventImpl))
+    assertThat(distributedRegion.validatedDestroy(new Object(), mockEntryEventImpl))
         .isSameAs(returnValue);
   }
 
@@ -96,7 +97,6 @@ public void cleanUpAfterFailedInitialImageHoldsLockForClear() {
 
   @Test
   public void cleanUpAfterFailedInitialImageDoesNotCloseEntriesIfIsPersistentRegionAndRecoveredFromDisk() {
-    DistributedRegion distributedRegion = mock(DistributedRegion.class);
     DiskRegion diskRegion = mock(DiskRegion.class);
 
     doCallRealMethod().when(distributedRegion).cleanUpAfterFailedGII(true);
@@ -111,7 +111,6 @@ public void cleanUpAfterFailedInitialImageDoesNotCloseEntriesIfIsPersistentRegio
 
   @Test
   public void lockHeldWhenRegionIsNotInitialized() {
-    DistributedRegion distributedRegion = mock(DistributedRegion.class);
     doCallRealMethod().when(distributedRegion).lockWhenRegionIsInitializing();
     when(distributedRegion.isInitialized()).thenReturn(false);
 
@@ -121,7 +120,6 @@ public void lockHeldWhenRegionIsNotInitialized() {
 
   @Test
   public void lockNotHeldWhenRegionIsInitialized() {
-    DistributedRegion distributedRegion = mock(DistributedRegion.class);
     doCallRealMethod().when(distributedRegion).lockWhenRegionIsInitializing();
     when(distributedRegion.isInitialized()).thenReturn(true);
 
@@ -131,7 +129,6 @@ public void lockNotHeldWhenRegionIsInitialized() {
 
   @Test
   public void versionHolderInvokesSetRegionSynchronizeScheduledIfVectorContainsLostMemberID() {
-    DistributedRegion distributedRegion = mock(DistributedRegion.class);
     when(distributedRegion.getVersionVector()).thenReturn(vector);
     when(vector.getHolderForMember(lostMemberVersionID)).thenReturn(holder);
     doCallRealMethod().when(distributedRegion).setRegionSynchronizeScheduled(lostMemberVersionID);
@@ -143,7 +140,6 @@ public void versionHolderInvokesSetRegionSynchronizeScheduledIfVectorContainsLos
 
   @Test
   public void versionHolderInvokesSetRegionSynchronizeScheduledOrDoneIfNotIfVectorContainsLostMemberID() {
-    DistributedRegion distributedRegion = mock(DistributedRegion.class);
     when(distributedRegion.getVersionVector()).thenReturn(vector);
     when(vector.getHolderForMember(lostMemberVersionID)).thenReturn(holder);
     doCallRealMethod().when(distributedRegion)
@@ -158,7 +154,6 @@ public void versionHolderInvokesSetRegionSynchronizeScheduledOrDoneIfNotIfVector
 
   @Test
   public void setRegionSynchronizedWithIfNotScheduledReturnsFalseIfVectorDoesNotContainLostMemberID() {
-    DistributedRegion distributedRegion = mock(DistributedRegion.class);
     when(distributedRegion.getVersionVector()).thenReturn(vector);
     when(vector.getHolderForMember(lostMemberVersionID)).thenReturn(holder);
 
@@ -170,7 +165,6 @@ public void setRegionSynchronizedWithIfNotScheduledReturnsFalseIfVectorDoesNotCo
 
   @Test
   public void regionSyncInvokedInPerformSynchronizeForLostMemberTaskAfterRegionInitialized() {
-    DistributedRegion distributedRegion = mock(DistributedRegion.class);
     when(distributedRegion.getDataPolicy()).thenReturn(mock(DataPolicy.class));
     when(distributedRegion.isInitializedWithWait()).thenReturn(true);
     doCallRealMethod().when(distributedRegion).performSynchronizeForLostMemberTask(member,
@@ -185,7 +179,6 @@ public void regionSyncInvokedInPerformSynchronizeForLostMemberTaskAfterRegionIni
 
   @Test
   public void regionSyncNotInvokedInPerformSynchronizeForLostMemberTaskIfRegionNotInitialized() {
-    DistributedRegion distributedRegion = mock(DistributedRegion.class);
     when(distributedRegion.getDataPolicy()).thenReturn(mock(DataPolicy.class));
     when(distributedRegion.isInitializedWithWait()).thenReturn(false);
     doCallRealMethod().when(distributedRegion).performSynchronizeForLostMemberTask(member,
@@ -201,7 +194,6 @@ public void validateAsynchronousEventDispatcherShouldDoNothingWhenDispatcherIdCa
     InternalCache internalCache = mock(InternalCache.class);
     when(internalCache.getAllGatewaySenders())
         .thenReturn(Collections.singleton(mock(GatewaySender.class)));
-    DistributedRegion distributedRegion = mock(DistributedRegion.class);
     when(distributedRegion.getCache()).thenReturn(internalCache);
     doCallRealMethod().when(distributedRegion).validateAsynchronousEventDispatcher(anyString());
 
@@ -216,7 +208,6 @@ public void validateAsynchronousEventDispatcherShouldDoNothingWhenFoundDispatche
     when(serialSender.getId()).thenReturn(senderId);
     InternalCache internalCache = mock(InternalCache.class);
     when(internalCache.getAllGatewaySenders()).thenReturn(Collections.singleton(serialSender));
-    DistributedRegion distributedRegion = mock(DistributedRegion.class);
     when(distributedRegion.getCache()).thenReturn(internalCache);
     doCallRealMethod().when(distributedRegion).validateAsynchronousEventDispatcher(anyString());
 
@@ -234,7 +225,6 @@ public void validateAsynchronousEventDispatcherShouldThrowExceptionWhenDispatche
     InternalCache internalCache = mock(InternalCache.class);
     when(internalCache.getAllGatewaySenders())
         .thenReturn(Collections.singleton(parallelAsyncEventQueue));
-    DistributedRegion distributedRegion = mock(DistributedRegion.class);
     when(distributedRegion.getCache()).thenReturn(internalCache);
     when(distributedRegion.getFullPath()).thenReturn(regionPath);
     doCallRealMethod().when(distributedRegion).validateAsynchronousEventDispatcher(anyString());
@@ -256,7 +246,6 @@ public void validateAsynchronousEventDispatcherShouldThrowExceptionWhenDispatche
     InternalCache internalCache = mock(InternalCache.class);
     when(internalCache.getAllGatewaySenders())
         .thenReturn(Collections.singleton(parallelGatewaySender));
-    DistributedRegion distributedRegion = mock(DistributedRegion.class);
     when(distributedRegion.getCache()).thenReturn(internalCache);
     when(distributedRegion.getFullPath()).thenReturn(regionPath);
     doCallRealMethod().when(distributedRegion).validateAsynchronousEventDispatcher(anyString());
@@ -269,7 +258,6 @@ public void validateAsynchronousEventDispatcherShouldThrowExceptionWhenDispatche
 
   @Test
   public void hasSeenEventDoesNotFindAndSetVersionTagIfFoundInEventTrackerAndVersionTagIsSet() {
-    DistributedRegion distributedRegion = mock(DistributedRegion.class);
     doCallRealMethod().when(distributedRegion).hasSeenEvent(event);
     when(distributedRegion.getEventTracker()).thenReturn(eventTracker);
     when(eventTracker.hasSeenEvent(event)).thenReturn(true);
@@ -282,7 +270,6 @@ public void hasSeenEventDoesNotFindAndSetVersionTagIfFoundInEventTrackerAndVersi
 
   @Test
   public void hasSeenEventDoesNotFindAndSetVersionTagIfFoundInEventTrackerAndConcurrencyChecksNotEnabled() {
-    DistributedRegion distributedRegion = mock(DistributedRegion.class);
     doCallRealMethod().when(distributedRegion).hasSeenEvent(event);
     when(distributedRegion.getEventTracker()).thenReturn(eventTracker);
     when(eventTracker.hasSeenEvent(event)).thenReturn(true);
@@ -296,7 +283,6 @@ public void hasSeenEventDoesNotFindAndSetVersionTagIfFoundInEventTrackerAndConcu
 
   @Test
   public void hasSeenEventDoesNotFindAndSetVersionTagIfFoundInEventTrackerAndEventIdIsNotSet() {
-    DistributedRegion distributedRegion = mock(DistributedRegion.class);
     doCallRealMethod().when(distributedRegion).hasSeenEvent(event);
     when(distributedRegion.getEventTracker()).thenReturn(eventTracker);
     when(eventTracker.hasSeenEvent(event)).thenReturn(true);
@@ -311,7 +297,6 @@ public void hasSeenEventDoesNotFindAndSetVersionTagIfFoundInEventTrackerAndEvent
 
   @Test
   public void hasSeenEventWillFindAndSetVersionTagIfFoundInEventTrackerButValidTagNotSet() {
-    DistributedRegion distributedRegion = mock(DistributedRegion.class);
     doCallRealMethod().when(distributedRegion).hasSeenEvent(event);
     when(distributedRegion.getEventTracker()).thenReturn(eventTracker);
     when(eventTracker.hasSeenEvent(event)).thenReturn(true);
@@ -326,7 +311,6 @@ public void hasSeenEventWillFindAndSetVersionTagIfFoundInEventTrackerButValidTag
 
   @Test
   public void hasSeenEventDoesNotFindAndSetVersionTagIfNotFoundEventInEventTrackerAndNotADuplicateEvent() {
-    DistributedRegion distributedRegion = mock(DistributedRegion.class);
     doCallRealMethod().when(distributedRegion).hasSeenEvent(event);
     when(distributedRegion.getEventTracker()).thenReturn(eventTracker);
     when(eventTracker.hasSeenEvent(event)).thenReturn(false);
@@ -338,7 +322,6 @@ public void hasSeenEventDoesNotFindAndSetVersionTagIfNotFoundEventInEventTracker
 
   @Test
   public void hasSeenEventDoesNotFindAndSetVersionTagIfNotFoundInEventTrackerAndConcurrencyChecksNotEnabled() {
-    DistributedRegion distributedRegion = mock(DistributedRegion.class);
     doCallRealMethod().when(distributedRegion).hasSeenEvent(event);
     when(distributedRegion.getEventTracker()).thenReturn(eventTracker);
     when(eventTracker.hasSeenEvent(event)).thenReturn(false);
@@ -352,7 +335,6 @@ public void hasSeenEventDoesNotFindAndSetVersionTagIfNotFoundInEventTrackerAndCo
 
   @Test
   public void hasSeenEventDoesNotFindAndSetVersionTagIfNotFoundInEventTrackerAndVersionTagIsSet() {
-    DistributedRegion distributedRegion = mock(DistributedRegion.class);
     doCallRealMethod().when(distributedRegion).hasSeenEvent(event);
     when(distributedRegion.getEventTracker()).thenReturn(eventTracker);
     when(eventTracker.hasSeenEvent(event)).thenReturn(false);
@@ -367,7 +349,6 @@ public void hasSeenEventDoesNotFindAndSetVersionTagIfNotFoundInEventTrackerAndVe
 
   @Test
   public void hasSeenEventDoesNotFindAndSetVersionTagIfNotFoundInEventTrackerAndNoEventId() {
-    DistributedRegion distributedRegion = mock(DistributedRegion.class);
     doCallRealMethod().when(distributedRegion).hasSeenEvent(event);
     when(distributedRegion.getEventTracker()).thenReturn(eventTracker);
     when(eventTracker.hasSeenEvent(event)).thenReturn(false);
@@ -382,7 +363,6 @@ public void hasSeenEventDoesNotFindAndSetVersionTagIfNotFoundInEventTrackerAndNo
 
   @Test
   public void hasSeenEventWillFindAndSetVersionTagIfNotFoundInEventTrackerAndIsPossibleDuplicateWithConcurrencyChecksEnabled() {
-    DistributedRegion distributedRegion = mock(DistributedRegion.class);
     doCallRealMethod().when(distributedRegion).hasSeenEvent(event);
     when(distributedRegion.getEventTracker()).thenReturn(eventTracker);
     when(eventTracker.hasSeenEvent(event)).thenReturn(false);

geode-core/src/test/java/org/apache/geode/internal/cache/persistence/PersistenceAdvisorImplTest.java

@@ -20,9 +20,11 @@
 import static java.util.Collections.singletonMap;
 import static org.apache.geode.cache.Region.SEPARATOR;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatCode;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyInt;
 import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.verify;
@@ -35,10 +37,11 @@
 import java.util.TreeSet;
 import java.util.UUID;
 
-import org.junit.Before;
-import org.junit.Test;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
 import org.mockito.ArgumentCaptor;
 
+import org.apache.geode.ForcedDisconnectException;
 import org.apache.geode.distributed.DistributedLockService;
 import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
 import org.apache.geode.internal.cache.CacheDistributionAdvisor;
@@ -69,13 +72,15 @@ public class PersistenceAdvisorImplTest {
   private PersistentStateQueryResults persistentStateQueryResults;
 
   private PersistenceAdvisorImpl persistenceAdvisorImpl;
+  private PersistentMemberManager persistentMemberManager;
 
   private int diskStoreIDIndex = 92837487; // some random number
 
-  @Before
+  @BeforeEach
   public void setUp() throws Exception {
     cacheDistributionAdvisor = mock(CacheDistributionAdvisor.class);
     persistentMemberView = mock(DiskRegion.class);
+    persistentMemberManager = mock(PersistentMemberManager.class);
     PersistentStateQueryMessageSenderFactory queryMessageSenderFactory =
         mock(PersistentStateQueryMessageSenderFactory.class);
     PersistentStateQueryMessage queryMessage =
@@ -91,10 +96,17 @@ public void setUp() throws Exception {
 
     persistenceAdvisorImpl =
         new PersistenceAdvisorImpl(cacheDistributionAdvisor, null, persistentMemberView, null, null,
-            null, mock(StartupStatus.class), mock(Transformer.class),
+            persistentMemberManager, mock(StartupStatus.class), mock(Transformer.class),
             mock(CollectionTransformer.class), queryMessageSenderFactory);
   }
 
+  @Test
+  void closeDoesNotThrowException() {
+    persistenceAdvisorImpl = spy(persistenceAdvisorImpl);
+    doThrow(new ForcedDisconnectException("test")).when(persistenceAdvisorImpl).releaseTieLock();
+    assertThatCode(() -> persistenceAdvisorImpl.close()).doesNotThrowAnyException();
+  }
+
   /**
    * GEODE-5402: This test creates a scenario where a member has two versions (based on timeStamp)
    * of another member. The call to getMembersToWaitFor should return that we wait for neither of