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 mhan...@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(); + } }