This is an automated email from the ASF dual-hosted git repository. bschuchardt pushed a commit to branch support/1.13 in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/support/1.13 by this push: new 8a66bc3 GEODE-8467: server fails to notify of a ForcedDisconnect and fails to tear down the cache (#5490) 8a66bc3 is described below commit 8a66bc3b376f391b7f387c74b9b5246d72882178 Author: Bruce Schuchardt <bschucha...@pivotal.io> AuthorDate: Tue Sep 1 07:32:47 2020 -0700 GEODE-8467: server fails to notify of a ForcedDisconnect and fails to tear down the cache (#5490) Catch exceptions that occur during XML generation and disable auto reconnect. Ensure that the DisconnectThread is launched by placing it in a "finally" block. (cherry picked from commit e402ed35102a4a885ad24a1052216b0542672bc7) --- .../geode/internal/cache/GemFireCacheImpl.java | 70 ++++++++++++++-------- .../geode/internal/cache/GemFireCacheImplTest.java | 12 ++++ .../internal/membership/gms/GMSMembership.java | 15 ++--- 3 files changed, 65 insertions(+), 32 deletions(-) diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java b/geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java index c4c8db5..2f6b0b4 100755 --- a/geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java +++ b/geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java @@ -59,6 +59,7 @@ import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; import java.io.OutputStreamWriter; +import java.io.PrintWriter; import java.io.Reader; import java.io.StringBufferInputStream; import java.io.StringWriter; @@ -1126,34 +1127,53 @@ public class GemFireCacheImpl implements InternalCache, InternalClientCache, Has @Override public void saveCacheXmlForReconnect() { - // there are two versions of this method so it can be unit-tested - boolean sharedConfigEnabled = getDistributionManager().getConfig().getUseSharedConfiguration(); + prepareForReconnect((pw) -> CacheXmlGenerator.generate((Cache) this, pw, false)); + } - if (!Boolean.getBoolean(GEMFIRE_PREFIX + "autoReconnect-useCacheXMLFile") - && !sharedConfigEnabled) { - try { - logger.info("generating XML to rebuild the cache after reconnect completes"); - StringPrintWriter pw = new StringPrintWriter(); - CacheXmlGenerator.generate((Cache) this, pw, false); - String cacheXML = pw.toString(); - getCacheConfig().setCacheXMLDescription(cacheXML); - logger.info("XML generation completed: {}", cacheXML); - } catch (CancelException e) { - logger.info("Unable to generate XML description for reconnect of cache due to exception", - e); - } - } else if (sharedConfigEnabled && !getCacheServers().isEmpty()) { - // we need to retain a cache-server description if this JVM was started by gfsh - List<CacheServerCreation> list = new ArrayList<>(getCacheServers().size()); - for (Object o : getCacheServers()) { - CacheServerImpl cs = (CacheServerImpl) o; - if (cs.isDefaultServer()) { - CacheServerCreation bsc = new CacheServerCreation(this, cs); - list.add(bsc); + + /** + * Testable version of saveCacheXmlForReconnect() that allows us to inject an XML generator + * + * @param xmlGenerator a consumer of a PrintWriter that generates a description of the Cache + */ + protected void prepareForReconnect(Consumer<PrintWriter> xmlGenerator) { + boolean sharedConfigEnabled = + getInternalDistributedSystem().getConfig().getUseSharedConfiguration(); + + try { + if (!Boolean.getBoolean(GEMFIRE_PREFIX + "autoReconnect-useCacheXMLFile") + && !sharedConfigEnabled) { + try { + logger.info("generating XML to rebuild the cache after reconnect completes"); + StringPrintWriter pw = new StringPrintWriter(); + xmlGenerator.accept(pw); + String cacheXML = pw.toString(); + getCacheConfig().setCacheXMLDescription(cacheXML); + logger.info("XML generation completed: {}", cacheXML); + } catch (CancelException e) { + logger.info("Unable to generate XML description for reconnect of cache due to exception", + e); } + } else if (sharedConfigEnabled && !getCacheServers().isEmpty()) { + // we need to retain a cache-server description if this JVM was started by gfsh + logger.info("saving cache server configuration for use with the cluster-configuration " + + "service on reconnect"); + List<CacheServerCreation> list = new ArrayList<>(getCacheServers().size()); + for (Object o : getCacheServers()) { + CacheServerImpl cs = (CacheServerImpl) o; + if (cs.isDefaultServer()) { + CacheServerCreation bsc = new CacheServerCreation(this, cs); + list.add(bsc); + } + } + getCacheConfig().setCacheServerCreation(list); + logger.info("cache server configuration saved"); } - getCacheConfig().setCacheServerCreation(list); - logger.info("CacheServer configuration saved"); + } catch (Throwable throwable) { + logger.info("Saving of cache configuration for auto-reconnect has failed. " + + "Auto-reconnect will be disabled since the cache cannot be rebuilt.", throwable); + getInternalDistributedSystem().getConfig().setDisableAutoReconnect(true); + } } diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/GemFireCacheImplTest.java b/geode-core/src/test/java/org/apache/geode/internal/cache/GemFireCacheImplTest.java index a0e7f28..cb36ebd 100644 --- a/geode-core/src/test/java/org/apache/geode/internal/cache/GemFireCacheImplTest.java +++ b/geode-core/src/test/java/org/apache/geode/internal/cache/GemFireCacheImplTest.java @@ -19,7 +19,9 @@ import static org.apache.geode.test.awaitility.GeodeAwaitility.await; 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.isA; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -84,6 +86,7 @@ public class GemFireCacheImplTest { typeRegistry = mock(TypeRegistry.class); DistributionConfig distributionConfig = mock(DistributionConfig.class); + when(distributionConfig.getUseSharedConfiguration()).thenReturn(false); DistributionManager distributionManager = mock(DistributionManager.class); ReplyProcessor21 replyProcessor21 = mock(ReplyProcessor21.class); @@ -620,6 +623,15 @@ public class GemFireCacheImplTest { .isSameAs(gemFireCacheImpl.getCacheServers()); } + @Test + public void cacheXmlGenerationErrorDisablesAutoReconnect() { + gemFireCacheImpl.prepareForReconnect((printWriter) -> { + throw new RuntimeException("error generating cache XML"); + }); + verify(internalDistributedSystem.getConfig()).setDisableAutoReconnect(Boolean.TRUE); + verify(cacheConfig, never()).setCacheXMLDescription(isA(String.class)); + } + @SuppressWarnings({"LambdaParameterHidesMemberVariable", "OverlyCoupledMethod", "unchecked"}) private GemFireCacheImpl gemFireCacheImpl(boolean useAsyncEventListeners) { return new GemFireCacheImpl( diff --git a/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java b/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java index db2ecae..1986931 100644 --- a/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java +++ b/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java @@ -2028,13 +2028,14 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID return; } - listener.saveConfig(); - - Thread reconnectThread = new LoggingThread("DisconnectThread", false, () -> { - lifecycleListener.forcedDisconnect(); - uncleanShutdown(reason, shutdownCause); - }); - reconnectThread.start(); + try { + listener.saveConfig(); + } finally { + new LoggingThread("DisconnectThread", false, () -> { + lifecycleListener.forcedDisconnect(); + uncleanShutdown(reason, shutdownCause); + }).start(); + } }