Shutdown state server and metric manager before service layer

Avoids a very small race condition window where metric update hooks
may point into the service layer components even after they have
been destroyed, as these are not explicitly unregistered today.

Another option would be to add unregistering on component destruction,
but that adds another race condition where an external client may
observe partial metric existence during this time window. By shutting
down the metric exporting interfaces first, we should avoid this.

4 files changed, 22 insertions, 1 deletions

diff --git a/metrics/src/vespa/metrics/metricmanager.cpp b/metrics/src/vespa/metrics/metricmanager.cpp
index 7ac912416de..84e92c2c316 100644
--- a/metrics/src/vespa/metrics/metricmanager.cpp
+++ b/metrics/src/vespa/metrics/metricmanager.cpp
@@ -94,6 +94,9 @@ MetricManager::~MetricManager()
 void
 MetricManager::stop()
 {
+    if (!running()) {
+        return; // Let stop() be idempotent.
+    }
     Runnable::stop();
     {
         vespalib::MonitorGuard sync(_waiter);
diff --git a/searchcore/src/apps/proton/proton.cpp b/searchcore/src/apps/proton/proton.cpp
index 8709b0d01b1..bc4329eadf6 100644
--- a/searchcore/src/apps/proton/proton.cpp
+++ b/searchcore/src/apps/proton/proton.cpp
@@ -252,6 +252,9 @@ App::Main()
                     }
                 }
             }
+            // Ensure metric manager and state server are shut down before we start tearing
+            // down any service layer components that they may end up transitively using.
+            protonUP->perform_pre_service_layer_shutdown_steps();
             if (spiProton) {
                 spiProton->getNode().requestShutdown("controlled shutdown");
                 spiProton->shutdown();
diff --git a/searchcore/src/vespa/searchcore/proton/server/proton.cpp b/searchcore/src/vespa/searchcore/proton/server/proton.cpp
index 1e579739d85..9d4c1527399 100644
--- a/searchcore/src/vespa/searchcore/proton/server/proton.cpp
+++ b/searchcore/src/vespa/searchcore/proton/server/proton.cpp
@@ -408,7 +408,7 @@ Proton::~Proton()
     _stateServer.reset();
     if (_metricsEngine) {
         _metricsEngine->removeMetricsHook(_metricsHook);
-        _metricsEngine->stop();
+        _metricsEngine->stop(); // Idempotent call
     }
     if (_matchEngine) {
         _matchEngine->close();
@@ -463,6 +463,15 @@ Proton::~Proton()
 }
 
 void
+Proton::perform_pre_service_layer_shutdown_steps()
+{
+    _stateServer.reset();
+    if (_metricsEngine) {
+        _metricsEngine->stop(); // Idempotent call
+    }
+}
+
+void
 Proton::closeDocumentDBs(vespalib::ThreadStackExecutorBase & executor) {
     // Need to extract names first as _documentDBMap is modified while removing.
     std::vector<DocTypeName> docTypes;
diff --git a/searchcore/src/vespa/searchcore/proton/server/proton.h b/searchcore/src/vespa/searchcore/proton/server/proton.h
index d5c1a8b7b78..d8d01e1806b 100644
--- a/searchcore/src/vespa/searchcore/proton/server/proton.h
+++ b/searchcore/src/vespa/searchcore/proton/server/proton.h
@@ -169,6 +169,12 @@ public:
      */
     BootstrapConfig::SP init();
 
+    /**
+     * Shuts down metric manager and state server functionality to avoid
+     * calls to these during service layer component tear-down.
+     */
+    void perform_pre_service_layer_shutdown_steps();
+
     // 2nd phase init: setup data structures.
     void init(const BootstrapConfig::SP & configSnapshot);