ensure that sending event to dispatch happens after updating internal state

* map.erase() invalidates reference to contents, move it to a local variable first
* cannot delete a ManagedRpcServer while we are in its callback

2 files changed, 56 insertions, 27 deletions

diff --git a/slobrok/src/vespa/slobrok/server/local_rpc_monitor_map.cpp b/slobrok/src/vespa/slobrok/server/local_rpc_monitor_map.cpp
index 3b517bb9241..5836118d37d 100644
--- a/slobrok/src/vespa/slobrok/server/local_rpc_monitor_map.cpp
+++ b/slobrok/src/vespa/slobrok/server/local_rpc_monitor_map.cpp
@@ -2,6 +2,7 @@
 
 #include "local_rpc_monitor_map.h"
 #include "sbenv.h"
+#include <vespa/fnet/frt/supervisor.h>
 #include <vespa/log/log.h>
 LOG_SETUP(".slobrok.server.local_rpc_monitor_map");
 
@@ -10,7 +11,8 @@ namespace slobrok {
 #pragma GCC diagnostic ignored "-Winline"
 
 LocalRpcMonitorMap::LocalRpcMonitorMap(FRT_Supervisor &supervisor)
-  : _map(),
+  : _delete(supervisor.GetScheduler()),
+    _map(),
     _dispatcher(),
     _history(),
     _supervisor(supervisor),
@@ -40,7 +42,7 @@ void LocalRpcMonitorMap::addLocal(const ServiceMapping &mapping,
         mapping.name.c_str(), mapping.spec.c_str());
     auto old = _map.find(mapping.name);
     if (old != _map.end()) {
-        PerService & exists = old->second;
+        const PerService & exists = old->second;
         if (exists.spec() == mapping.spec) {
             LOG(debug, "added mapping %s->%s was already present",
                 mapping.name.c_str(), mapping.spec.c_str());
@@ -70,17 +72,19 @@ void LocalRpcMonitorMap::add(const ServiceMapping &mapping) {
             exists.localOnly = false;
             return;
         }
+        PerService removed = std::move(exists);
+        _map.erase(old);
         LOG(warning, "added mapping %s->%s, but already had conflicting mapping %s->%s",
             mapping.name.c_str(), mapping.spec.c_str(),
-            exists.name().c_str(), exists.spec().c_str());
-        if (exists.up) {
-            _dispatcher.remove(exists.mapping());
-        }
-        if (exists.inflight) {
-            auto target = std::move(exists.inflight);
+            removed.name().c_str(), removed.spec().c_str());
+        if (removed.inflight) {
+            auto target = std::move(removed.inflight);
             target->doneHandler(OkState(13, "conflict during initialization"));
         }
-        _map.erase(old);
+        if (removed.up) {
+            _dispatcher.remove(removed.mapping());
+        }
+        _delete.later(std::move(removed.srv));
     }
     auto [ iter, was_inserted ] =
         _map.try_emplace(mapping.name, globalService(mapping));
@@ -90,23 +94,23 @@ void LocalRpcMonitorMap::add(const ServiceMapping &mapping) {
 void LocalRpcMonitorMap::remove(const ServiceMapping &mapping) {
     auto iter = _map.find(mapping.name);
     if (iter != _map.end()) {
+        PerService removed = std::move(iter->second);
+        _map.erase(iter);
         LOG(debug, "remove: mapping %s->%s", mapping.name.c_str(), mapping.spec.c_str());
-        PerService & exists = iter->second;
-        if (mapping.spec != exists.spec()) {
+        if (mapping.spec != removed.spec()) {
             LOG(warning, "inconsistent specs for name '%s': had '%s', but was asked to remove '%s'",
                 mapping.name.c_str(),
-                exists.spec().c_str(),
+                removed.spec().c_str(),
                 mapping.spec.c_str());
-            return;
-        }
-        if (exists.up) {
-            _dispatcher.remove(exists.mapping());
         }
-        if (exists.inflight) {
-            auto target = std::move(exists.inflight);
+        if (removed.inflight) {
+            auto target = std::move(removed.inflight);
             target->doneHandler(OkState(13, "removed during initialization"));
         }
-        _map.erase(iter);
+        if (removed.up) {
+            _dispatcher.remove(removed.mapping());
+        }
+        _delete.later(std::move(removed.srv));
     } else {
         LOG(debug, "tried to remove non-existing mapping %s->%s",
             mapping.name.c_str(), mapping.spec.c_str());
@@ -120,13 +124,17 @@ void LocalRpcMonitorMap::notifyFailedRpcSrv(ManagedRpcServer *rpcsrv, std::strin
         auto target = std::move(psd.inflight);
         target->doneHandler(OkState(13, "failed check using listNames callback"));
     }
-    if (psd.up) {
-        psd.up = false;
-        _dispatcher.remove(psd.mapping());
-    }
     if (psd.localOnly) {
-        auto iter = _map.find(psd.name());
+        PerService removed = std::move(psd);
+        auto iter = _map.find(removed.name());
         _map.erase(iter);
+        if (removed.up) {
+            _dispatcher.remove(removed.mapping());
+        }
+        _delete.later(std::move(removed.srv));
+    } else if (psd.up) {
+        psd.up = false;
+        _dispatcher.remove(psd.mapping());
     }
 }
 
diff --git a/slobrok/src/vespa/slobrok/server/local_rpc_monitor_map.h b/slobrok/src/vespa/slobrok/server/local_rpc_monitor_map.h
index 3a8d858e382..8ca876df6b0 100644
--- a/slobrok/src/vespa/slobrok/server/local_rpc_monitor_map.h
+++ b/slobrok/src/vespa/slobrok/server/local_rpc_monitor_map.h
@@ -29,15 +29,36 @@ class LocalRpcMonitorMap : public IRpcServerManager,
                            public MapListener
 {
 private:
+    class DeleteTask : public FNET_Task {
+        using MUP = std::unique_ptr<ManagedRpcServer>;
+        std::vector<MUP> _deleteList;
+    public:
+        void later(MUP rpcsrv) {
+            _deleteList.emplace_back(std::move(rpcsrv));
+            ScheduleNow();
+        }
+        void PerformTask() override {
+            std::vector<MUP> deleteAfterSwap;
+            std::swap(deleteAfterSwap, _deleteList);
+        }
+        DeleteTask(FNET_Scheduler *scheduler)
+          : FNET_Task(scheduler),
+            _deleteList()
+        {}
+        ~DeleteTask() { Kill(); }
+    };
+
+    DeleteTask _delete;
+
     struct PerService {
         bool up;
         bool localOnly;
         std::unique_ptr<ScriptCommand> inflight;
         std::unique_ptr<ManagedRpcServer> srv;
 
-        vespalib::string name() { return srv->getName(); }
-        vespalib::string spec() { return srv->getSpec(); }
-        ServiceMapping mapping() { return ServiceMapping{srv->getName(), srv->getSpec()}; }
+        vespalib::string name() const { return srv->getName(); }
+        vespalib::string spec() const { return srv->getSpec(); }
+        ServiceMapping mapping() const { return ServiceMapping{srv->getName(), srv->getSpec()}; }
     };
 
     std::unique_ptr<ManagedRpcServer> managedFor(const ServiceMapping &mapping) {