diff options
author | Bjørn Christian Seime <bjorn.christian@seime.no> | 2022-02-07 11:56:37 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2022-02-07 11:56:37 +0100 |
commit | 395d83f29a1d270c2d29a5226429d21c34465327 (patch) | |
tree | d6556906d4c61e6b25572c5978e328be72997292 | |
parent | ce128399b3c29f0b9178f45fb522efb5ff631fe3 (diff) |
Revert "Shutdown reconfiguration thread in a more controlled way [run-systemtest]"
5 files changed, 50 insertions, 58 deletions
diff --git a/container-core/src/main/java/com/yahoo/container/core/config/HandlersConfigurerDi.java b/container-core/src/main/java/com/yahoo/container/core/config/HandlersConfigurerDi.java index decb0513e26..bcd3665a3d6 100644 --- a/container-core/src/main/java/com/yahoo/container/core/config/HandlersConfigurerDi.java +++ b/container-core/src/main/java/com/yahoo/container/core/config/HandlersConfigurerDi.java @@ -145,8 +145,6 @@ public class HandlersConfigurerDi { public void shutdown() { container.shutdown(currentGraph); } - public void shutdownConfigRetriever() { container.shutdownConfigRetriever(); } - /** Returns the currently active application configuration generation */ public long generation() { return currentGraph.generation(); } diff --git a/container-core/src/main/java/com/yahoo/container/di/Container.java b/container-core/src/main/java/com/yahoo/container/di/Container.java index d800851c728..2ef520ea4e9 100644 --- a/container-core/src/main/java/com/yahoo/container/di/Container.java +++ b/container-core/src/main/java/com/yahoo/container/di/Container.java @@ -266,14 +266,14 @@ public class Container { } public void shutdown(ComponentGraph graph) { - shutdownConfigRetriever(); + shutdownConfigurer(); if (graph != null) { scheduleGraphForDeconstruction(graph); destructor.shutdown(); } } - public void shutdownConfigRetriever() { + void shutdownConfigurer() { retriever.shutdown(); } diff --git a/container-core/src/test/java/com/yahoo/container/di/ContainerTest.java b/container-core/src/test/java/com/yahoo/container/di/ContainerTest.java index 74c08b1044b..1570c529cdf 100644 --- a/container-core/src/test/java/com/yahoo/container/di/ContainerTest.java +++ b/container-core/src/test/java/com/yahoo/container/di/ContainerTest.java @@ -44,7 +44,7 @@ public class ContainerTest extends ContainerTestBase { ComponentTakingConfig component = createComponentTakingConfig(getNewComponentGraph(container)); assertEquals("myString", component.config.stringVal()); - container.shutdownConfigRetriever(); + container.shutdownConfigurer(); } @Test @@ -66,7 +66,7 @@ public class ContainerTest extends ContainerTestBase { ComponentTakingConfig component2 = createComponentTakingConfig(newComponentGraph); assertEquals("reconfigured", component2.config.stringVal()); - container.shutdownConfigRetriever(); + container.shutdownConfigurer(); } @Test @@ -90,7 +90,7 @@ public class ContainerTest extends ContainerTestBase { assertNotNull(ComponentGraph.getNode(newGraph, "id1")); assertNotNull(ComponentGraph.getNode(newGraph, "id2")); - container.shutdownConfigRetriever(); + container.shutdownConfigurer(); } //@Test TODO diff --git a/container-disc/src/main/java/com/yahoo/container/jdisc/ConfiguredApplication.java b/container-disc/src/main/java/com/yahoo/container/jdisc/ConfiguredApplication.java index 64e94bd5f03..24dcf18b555 100644 --- a/container-disc/src/main/java/com/yahoo/container/jdisc/ConfiguredApplication.java +++ b/container-disc/src/main/java/com/yahoo/container/jdisc/ConfiguredApplication.java @@ -94,15 +94,14 @@ public final class ConfiguredApplication implements Application { new ComponentRegistry<>()); private final OsgiFramework restrictedOsgiFramework; private final Phaser nonTerminatedContainerTracker = new Phaser(1); - private final Thread reconfigurerThread; - private final Thread portWatcher; private HandlersConfigurerDi configurer; + private Thread reconfigurerThread; + private Thread portWatcher; private QrConfig qrConfig; private Register slobrokRegistrator = null; private Supervisor supervisor = null; private Acceptor acceptor = null; - private volatile boolean shutdownReconfiguration = false; static { LogSetup.initVespaLogging("Container"); @@ -136,8 +135,6 @@ public final class ConfiguredApplication implements Application { : Optional.empty(); this.restrictedOsgiFramework = new DisableOsgiFramework(new RestrictedBundleContext(osgiFramework.bundleContext())); this.shutdownDeadline = new ShutdownDeadline(configId); - this.reconfigurerThread = new Thread(this::doReconfigurationLoop, "configured-application-reconfigurer"); - this.portWatcher = new Thread(this::watchPortChange, "configured-application-port-watcher"); } @Override @@ -149,9 +146,8 @@ public final class ConfiguredApplication implements Application { ContainerBuilder builder = createBuilderWithGuiceBindings(); configurer = createConfigurer(builder.guiceModules().activate()); initializeAndActivateContainer(builder, () -> {}); - reconfigurerThread.setDaemon(true); - reconfigurerThread.start(); - + startReconfigurerThread(); + portWatcher = new Thread(this::watchPortChange, "configured-application-port-watcher"); portWatcher.setDaemon(true); portWatcher.start(); if (setupRpc()) { @@ -290,27 +286,30 @@ public final class ConfiguredApplication implements Application { } @SuppressWarnings("removal") // TODO Vespa 8: remove - private void doReconfigurationLoop() { - while (!shutdownReconfiguration) { - try { - ContainerBuilder builder = createBuilderWithGuiceBindings(); - - // Block until new config arrives, and it should be applied - Runnable cleanupTask = configurer.waitForNextGraphGeneration(builder.guiceModules().activate(), false); - initializeAndActivateContainer(builder, cleanupTask); - } catch (UncheckedInterruptedException | ConfigInterruptedException e) { - break; - } catch (Exception | LinkageError e) { // LinkageError: OSGi problems - tryReportFailedComponentGraphConstructionMetric(configurer, e); - log.log(Level.SEVERE, - "Reconfiguration failed, your application package must be fixed, unless this is a " + - "JNI reload issue: " + Exceptions.toMessageString(e), e); - } catch (Error e) { - com.yahoo.protect.Process.logAndDie("java.lang.Error on reconfiguration: We are probably in " + - "a bad state and will terminate", e); + private void startReconfigurerThread() { + reconfigurerThread = new Thread(() -> { + while ( ! Thread.interrupted()) { + try { + ContainerBuilder builder = createBuilderWithGuiceBindings(); + + // Block until new config arrives, and it should be applied + Runnable cleanupTask = configurer.waitForNextGraphGeneration(builder.guiceModules().activate(), false); + initializeAndActivateContainer(builder, cleanupTask); + } catch (UncheckedInterruptedException | ConfigInterruptedException e) { + break; + } catch (Exception | LinkageError e) { // LinkageError: OSGi problems + tryReportFailedComponentGraphConstructionMetric(configurer, e); + log.log(Level.SEVERE, + "Reconfiguration failed, your application package must be fixed, unless this is a " + + "JNI reload issue: " + Exceptions.toMessageString(e), e); + } catch (Error e) { + com.yahoo.protect.Process.logAndDie("java.lang.Error on reconfiguration: We are probably in " + + "a bad state and will terminate", e); + } } - } - log.fine("Reconfiguration loop exited"); + log.fine("Shutting down HandlersConfigurerDi"); + }, "configured-application-reconfigurer"); + reconfigurerThread.start(); } private static void tryReportFailedComponentGraphConstructionMetric(HandlersConfigurerDi configurer, Throwable error) { @@ -398,7 +397,7 @@ public final class ConfiguredApplication implements Application { } private void stopServersAndAwaitTermination(String logPrefix) { - shutdownReconfigurer(); + shutdownReconfigurerThread(); log.info(logPrefix + ": Closing servers"); startAndStopServers(List.of()); startAndRemoveClients(List.of()); @@ -408,24 +407,20 @@ public final class ConfiguredApplication implements Application { log.info(logPrefix + ": Finished"); } - private void shutdownReconfigurer() { - if (!reconfigurerThread.isAlive()) { - log.info("Reconfiguration thread shutdown already completed"); - return; - } - log.info("Shutting down reconfiguration thread"); - long start = System.currentTimeMillis(); - shutdownReconfiguration = true; - configurer.shutdownConfigRetriever(); + // TODO Do more graceful shutdown of reconfigurer thread. The interrupt may leave the container in state where + // graceful shutdown is impossible or may hang. + private void shutdownReconfigurerThread() { try { - reconfigurerThread.join(); - log.info(String.format( - "Reconfiguration shutdown completed in %.3f seconds", (System.currentTimeMillis() - start) / 1000D)); + //Workaround for component constructors masking InterruptedException. + while (reconfigurerThread != null && reconfigurerThread.isAlive()) { + reconfigurerThread.interrupt(); + long millis = 200; + reconfigurerThread.join(millis); + reconfigurerThread = null; + } } catch (InterruptedException e) { - String message = "Interrupted while waiting for reconfiguration shutdown"; - log.warning(message); - log.log(Level.FINE, e.getMessage(), e); - throw new UncheckedInterruptedException(message, true); + log.info("Interrupted while joining on HandlersConfigurer reconfigure thread."); + Thread.currentThread().interrupt(); } } diff --git a/standalone-container/src/main/java/com/yahoo/container/standalone/StandaloneSubscriberFactory.java b/standalone-container/src/main/java/com/yahoo/container/standalone/StandaloneSubscriberFactory.java index 907c9649912..11f799fcfd4 100644 --- a/standalone-container/src/main/java/com/yahoo/container/standalone/StandaloneSubscriberFactory.java +++ b/standalone-container/src/main/java/com/yahoo/container/standalone/StandaloneSubscriberFactory.java @@ -31,7 +31,6 @@ public class StandaloneSubscriberFactory implements SubscriberFactory { private final Set<ConfigKey<ConfigInstance>> configKeys; private long generation = -1L; - private volatile boolean shutdown = false; StandaloneSubscriber(Set<ConfigKey<ConfigInstance>> configKeys) { this.configKeys = configKeys; @@ -42,7 +41,9 @@ public class StandaloneSubscriberFactory implements SubscriberFactory { return generation == 0; } - @Override public void close() { shutdown = true; } + @Override + public void close() { + } @Override public Map<ConfigKey<ConfigInstance>, ConfigInstance> config() { @@ -63,11 +64,9 @@ public class StandaloneSubscriberFactory implements SubscriberFactory { if (generation != 0) { try { - while (!shutdown && !Thread.interrupted()) { - Thread.sleep(100); + while (!Thread.interrupted()) { + Thread.sleep(10000); } - if (shutdown) // Same semantics as an actual interrupt - throw new ConfigInterruptedException(new InterruptedException()); } catch (InterruptedException e) { throw new ConfigInterruptedException(e); } |