GEODE-7297: Handle more exceptions in FederatingManager (apache#4208)

Handle or prevent CancelException and NullPointerException in removeMemberArtifacts.
ringles · Oct 24, 2019 · 0df32bd · 0df32bd
1 parent 6ae3f0b
commit 0df32bd
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 6 deletions.
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/FederatingManager.java b/geode-core/src/main/java/org/apache/geode/management/internal/FederatingManager.java
@@ -326,12 +326,16 @@ void removeMemberArtifacts(DistributedMember member, boolean crashed) {
         // ignored
       }
       try {
-        monitoringRegion.localDestroyRegion();
+        if (monitoringRegion != null) {
+          monitoringRegion.localDestroyRegion();
+        }
       } catch (CancelException | RegionDestroyedException ignore) {
         // ignored
       }
       try {
-        notificationRegion.localDestroyRegion();
+        if (notificationRegion != null) {
+          notificationRegion.localDestroyRegion();
+        }
       } catch (CancelException | RegionDestroyedException ignore) {
         // ignored
       }

diff --git a/geode-core/src/test/java/org/apache/geode/management/internal/FederatingManagerTest.java b/geode-core/src/test/java/org/apache/geode/management/internal/FederatingManagerTest.java
@@ -15,6 +15,7 @@
 package org.apache.geode.management.internal;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.catchThrowable;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.doThrow;
@@ -250,8 +251,6 @@ public void removeMemberArtifactsDoesNotThrowIfMBeanProxyFactoryThrowsRegionDest
         .removeAllProxies(member, monitoringRegion);
   }
 
-  // ----------
-
   @Test
   public void removeMemberArtifactsProxyFactoryDoesNotThrowIfCacheClosed() {
     InternalDistributedMember member = member();
@@ -316,8 +315,6 @@ public void removeMemberArtifactsMonitoringRegionDoesNotThrowIfCacheClosed() {
         .localDestroyRegion();
   }
 
-  // ----------
-
   @Test
   public void removeMemberArtifactsProxyFactoryDoesNotThrowIfSystemDisconnected() {
     InternalDistributedMember member = member();
@@ -382,6 +379,42 @@ public void removeMemberArtifactsMonitoringRegionDoesNotThrowIfSystemDisconnecte
         .localDestroyRegion();
   }
 
+  @Test
+  public void removeMemberArtifactsDoesNotThrowIfNotificationRegionIsNull() {
+    InternalDistributedMember member = member();
+    when(repo.getEntryFromMonitoringRegionMap(eq(member)))
+        .thenReturn(mock(Region.class));
+    when(repo.getEntryFromNotifRegionMap(eq(member)))
+        .thenReturn(null);
+    when(system.getDistributedMember())
+        .thenReturn(member);
+    FederatingManager federatingManager = new FederatingManager(repo, system, service, cache,
+        statisticsFactory, statisticsClock, proxyFactory, messenger, executorService);
+
+    Throwable thrown = catchThrowable(() -> federatingManager.removeMemberArtifacts(member, false));
+
+    assertThat(thrown)
+        .isNull();
+  }
+
+  @Test
+  public void removeMemberArtifactsDoesNotThrowIfMonitoringRegionIsNull() {
+    InternalDistributedMember member = member();
+    when(repo.getEntryFromMonitoringRegionMap(eq(member)))
+        .thenReturn(null);
+    when(repo.getEntryFromNotifRegionMap(eq(member)))
+        .thenReturn(mock(Region.class));
+    when(system.getDistributedMember())
+        .thenReturn(member);
+    FederatingManager federatingManager = new FederatingManager(repo, system, service, cache,
+        statisticsFactory, statisticsClock, proxyFactory, messenger, executorService);
+
+    Throwable thrown = catchThrowable(() -> federatingManager.removeMemberArtifacts(member, false));
+
+    assertThat(thrown)
+        .isNull();
+  }
+
   private InternalDistributedMember member() {
     return member(1, 1);
   }