diff options
author | Arne Juul <arnej@verizonmedia.com> | 2021-08-17 11:59:31 +0000 |
---|---|---|
committer | Arne Juul <arnej@verizonmedia.com> | 2021-08-17 14:05:16 +0000 |
commit | 0fd9e136bfe209f9cbfaa1f0c14e2ffc5834a772 (patch) | |
tree | e12a6aa9fd7342b492fb6a2dd1f373751a98ee71 /slobrok | |
parent | 1ec01c1db964c6a0c3553a6e3c1eb8cd9700413d (diff) |
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
Diffstat (limited to 'slobrok')
-rw-r--r-- | slobrok/src/vespa/slobrok/server/local_rpc_monitor_map.cpp | 56 | ||||
-rw-r--r-- | slobrok/src/vespa/slobrok/server/local_rpc_monitor_map.h | 27 |
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) { |