[geode] branch release/1.10.0 updated: GEODE-6959: Prevent NPE in GMSMembershipManager for null AlertAppender (#3899)

[kl...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

klund pushed a commit to branch release/1.10.0
in repository https://gitbox.apache.org/repos/asf/geode.git


The following commit(s) were added to refs/heads/release/1.10.0 by this push:
     new bb509b0  GEODE-6959: Prevent NPE in GMSMembershipManager for null AlertAppender (#3899)
bb509b0 is described below

commit bb509b0e283917e94a4f6046791e59516f1320a4
Author: Kirk Lund <kl...@apache.org>
AuthorDate: Thu Aug 8 14:59:44 2019 -0700

    GEODE-6959: Prevent NPE in GMSMembershipManager for null AlertAppender (#3899)
    
    If a custom log4j2.xml is used without specifying the Geode AlertAppender,
    GMSMembershipManager may throw a NullPointerException when invoking
    AlertAppender.getInstance().stopSession() during a forceDisconnect. This
    change prevents the NullPointerException allowing forceDisconnect to finish.
    
    Users using Spring Boot with Logback are more likely to hit this bug.
    
    Co-authored-by: Mark Hanson mhanson@pivotal.io
    
    (cherry picked from commit dd15fec1f2ecbc3bc0cdfc42072252c379e0bb89)
---
 .../membership/gms/mgr/GMSMembershipManager.java   |  2 +-
 .../internal/logging/log4j/AlertAppender.java      | 16 ++++++++++++-
 .../internal/logging/log4j/AlertAppenderTest.java  | 26 ++++++++++++++++++++++
 3 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/mgr/GMSMembershipManager.java b/geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/mgr/GMSMembershipManager.java
index bd70990..8411ee6 100644
--- a/geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/mgr/GMSMembershipManager.java
+++ b/geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/mgr/GMSMembershipManager.java
@@ -2509,7 +2509,7 @@ public class GMSMembershipManager implements MembershipManager, Manager {
     services.setShutdownCause(shutdownCause);
     services.getCancelCriterion().cancel(reason);
 
-    AlertAppender.getInstance().stopSession();
+    AlertAppender.stopSessionIfRunning();
 
     if (!inhibitForceDisconnectLogging) {
       logger.fatal(
diff --git a/geode-core/src/main/java/org/apache/geode/internal/logging/log4j/AlertAppender.java b/geode-core/src/main/java/org/apache/geode/internal/logging/log4j/AlertAppender.java
index d56a0da..40332da 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/logging/log4j/AlertAppender.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/logging/log4j/AlertAppender.java
@@ -36,6 +36,7 @@ import org.apache.logging.log4j.core.config.plugins.Plugin;
 import org.apache.logging.log4j.core.config.plugins.PluginBuilderAttribute;
 import org.apache.logging.log4j.core.config.plugins.PluginBuilderFactory;
 
+import org.apache.geode.annotations.VisibleForTesting;
 import org.apache.geode.annotations.internal.MakeNotStatic;
 import org.apache.geode.distributed.DistributedMember;
 import org.apache.geode.internal.alerting.AlertLevel;
@@ -337,7 +338,20 @@ public class AlertAppender extends AbstractAppender
     return listeners;
   }
 
-  public static AlertAppender getInstance() {
+  @VisibleForTesting
+  static AlertAppender getInstance() {
     return instanceRef.get();
   }
+
+  @VisibleForTesting
+  static void setInstance(AlertAppender alertAppender) {
+    instanceRef.set(alertAppender);
+  }
+
+  public static void stopSessionIfRunning() {
+    AlertAppender instance = instanceRef.get();
+    if (instance != null) {
+      instance.stopSession();
+    }
+  }
 }
diff --git a/geode-core/src/test/java/org/apache/geode/internal/logging/log4j/AlertAppenderTest.java b/geode-core/src/test/java/org/apache/geode/internal/logging/log4j/AlertAppenderTest.java
index 0ba7e45..6779b20 100644
--- a/geode-core/src/test/java/org/apache/geode/internal/logging/log4j/AlertAppenderTest.java
+++ b/geode-core/src/test/java/org/apache/geode/internal/logging/log4j/AlertAppenderTest.java
@@ -15,9 +15,13 @@
 package org.apache.geode.internal.logging.log4j;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatCode;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
 
 import org.apache.logging.log4j.Level;
+import org.junit.After;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
@@ -52,6 +56,11 @@ public class AlertAppenderTest {
     asAlertingProvider = alertAppender;
   }
 
+  @After
+  public void tearDown() {
+    AlertAppender.setInstance(null);
+  }
+
   @Test
   public void alertListenersIsEmptyByDefault() {
     assertThat(alertAppender.getAlertListeners()).isEmpty();
@@ -164,4 +173,21 @@ public class AlertAppenderTest {
     assertThat(alertAppender.getAlertListeners()).containsExactly(listener1, listener2,
         listener3);
   }
+
+  @Test
+  public void stopSessionIfRunningDoesNotThrowIfReferenceIsNull() {
+    AlertAppender.setInstance(null);
+
+    assertThatCode(AlertAppender::stopSessionIfRunning).doesNotThrowAnyException();
+  }
+
+  @Test
+  public void stopSessionIfRunningStopCurrentInstance() {
+    alertAppender = spy(alertAppender);
+    AlertAppender.setInstance(alertAppender);
+
+    AlertAppender.stopSessionIfRunning();
+
+    verify(alertAppender).stopSession();
+  }
 }