simplify request completion handling

7 files changed, 77 insertions, 31 deletions

diff --git a/slobrok/src/tests/local_rpc_monitor_map/local_rpc_monitor_map_test.cpp b/slobrok/src/tests/local_rpc_monitor_map/local_rpc_monitor_map_test.cpp
index 9782f6ccbdc..46f6837c4f1 100644
--- a/slobrok/src/tests/local_rpc_monitor_map/local_rpc_monitor_map_test.cpp
+++ b/slobrok/src/tests/local_rpc_monitor_map/local_rpc_monitor_map_test.cpp
@@ -164,7 +164,7 @@ struct LocalRpcMonitorMapTest : public ::testing::Test {
 };
 LocalRpcMonitorMapTest::~LocalRpcMonitorMapTest() = default;
 
-struct MyAddLocalHandler : LocalRpcMonitorMap::AddLocalCompletionHandler {
+struct MyAddLocalHandler : CompletionHandler {
     std::unique_ptr<OkState> &state;
     bool &handler_deleted;
     MyAddLocalHandler(std::unique_ptr<OkState> &s, bool &hd)
diff --git a/slobrok/src/vespa/slobrok/server/CMakeLists.txt b/slobrok/src/vespa/slobrok/server/CMakeLists.txt
index b4e7d5de1d5..ae1a05c5181 100644
--- a/slobrok/src/vespa/slobrok/server/CMakeLists.txt
+++ b/slobrok/src/vespa/slobrok/server/CMakeLists.txt
@@ -20,6 +20,7 @@ vespa_add_library(slobrok_slobrokserver
     reconfigurable_stateserver.cpp
     remote_check.cpp
     remote_slobrok.cpp
+    request_completion_handler.cpp
     reserved_name.cpp
     rpc_mapping_monitor.cpp
     rpc_server_manager.cpp
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 16e47371cbb..99cf59e0440 100644
--- a/slobrok/src/vespa/slobrok/server/local_rpc_monitor_map.cpp
+++ b/slobrok/src/vespa/slobrok/server/local_rpc_monitor_map.cpp
@@ -12,12 +12,12 @@ namespace slobrok {
 
 namespace {
 
-struct ChainedAddLocalCompletionHandler : LocalRpcMonitorMap::AddLocalCompletionHandler {
-    std::unique_ptr<AddLocalCompletionHandler> first;
-    std::unique_ptr<AddLocalCompletionHandler> second;
+struct ChainedCompletionHandler : CompletionHandler {
+    std::unique_ptr<CompletionHandler> first;
+    std::unique_ptr<CompletionHandler> second;
 
-    ChainedAddLocalCompletionHandler(std::unique_ptr<AddLocalCompletionHandler> f,
-                                     std::unique_ptr<AddLocalCompletionHandler> s)
+    ChainedCompletionHandler(std::unique_ptr<CompletionHandler> f,
+                                     std::unique_ptr<CompletionHandler> s)
         : first(std::move(f)), second(std::move(s))
     {}
 
@@ -25,7 +25,7 @@ struct ChainedAddLocalCompletionHandler : LocalRpcMonitorMap::AddLocalCompletion
         first->doneHandler(result);
         second->doneHandler(result);
     }
-    ~ChainedAddLocalCompletionHandler() override {}
+    ~ChainedCompletionHandler() override {}
 };
 
 }
@@ -111,7 +111,7 @@ bool LocalRpcMonitorMap::wouldConflict(const ServiceMapping &mapping) const {
 }
 
 void LocalRpcMonitorMap::addLocal(const ServiceMapping &mapping,
-                                  std::unique_ptr<AddLocalCompletionHandler> inflight)
+                                  std::unique_ptr<CompletionHandler> inflight)
 {
     LOG(debug, "try local add: mapping %s->%s",
         mapping.name.c_str(), mapping.spec.c_str());
@@ -124,7 +124,7 @@ void LocalRpcMonitorMap::addLocal(const ServiceMapping &mapping,
             if (exists.up) {
                 inflight->doneHandler(OkState(0, "already registered"));
             } else if (exists.inflight) {
-                auto newInflight = std::make_unique<ChainedAddLocalCompletionHandler>(
+                auto newInflight = std::make_unique<ChainedCompletionHandler>(
                     std::move(exists.inflight),
                     std::move(inflight));
                 exists.inflight = std::move(newInflight);
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 e3d081eacc9..96c0ee03245 100644
--- a/slobrok/src/vespa/slobrok/server/local_rpc_monitor_map.h
+++ b/slobrok/src/vespa/slobrok/server/local_rpc_monitor_map.h
@@ -8,6 +8,7 @@
 #include "mapping_monitor.h"
 #include "named_service.h"
 #include "proxy_map_source.h"
+#include "request_completion_handler.h"
 #include "service_map_history.h"
 #include "service_mapping.h"
 
@@ -27,13 +28,6 @@ namespace slobrok {
 class LocalRpcMonitorMap : public MapListener,
                            public MappingMonitorOwner
 {
-public:
-    // Interface used to signal the result of addLocal
-    struct AddLocalCompletionHandler {
-        virtual void doneHandler(OkState result) = 0;
-        virtual ~AddLocalCompletionHandler() {}
-    };
-
 private:
     enum class EventType { ADD, REMOVE };
 
@@ -73,12 +67,12 @@ private:
     struct PerService {
         bool up;
         bool localOnly;
-        std::unique_ptr<AddLocalCompletionHandler> inflight;
+        std::unique_ptr<CompletionHandler> inflight;
         vespalib::string spec;
     };
 
     PerService localService(const ServiceMapping &mapping,
-                            std::unique_ptr<AddLocalCompletionHandler> inflight)
+                            std::unique_ptr<CompletionHandler> inflight)
     {
         return PerService{
             .up = false,
@@ -116,7 +110,7 @@ private:
         ServiceMapping mapping;
         bool up;
         bool localOnly;
-        std::unique_ptr<AddLocalCompletionHandler> inflight;
+        std::unique_ptr<CompletionHandler> inflight;
     };
 
     RemovedData removeFromMap(Map::iterator iter);
@@ -133,7 +127,7 @@ public:
 
     /** for use by register API, will call doneHandler() on inflight script */
     void addLocal(const ServiceMapping &mapping,
-                  std::unique_ptr<AddLocalCompletionHandler> inflight);
+                  std::unique_ptr<CompletionHandler> inflight);
 
     /** for use by unregister API */
     void removeLocal(const ServiceMapping &mapping);
diff --git a/slobrok/src/vespa/slobrok/server/request_completion_handler.cpp b/slobrok/src/vespa/slobrok/server/request_completion_handler.cpp
new file mode 100644
index 00000000000..b4c744ad447
--- /dev/null
+++ b/slobrok/src/vespa/slobrok/server/request_completion_handler.cpp
@@ -0,0 +1,30 @@
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+
+#include "request_completion_handler.h"
+#include <vespa/fnet/frt/rpcrequest.h>
+
+namespace slobrok {
+
+RequestCompletionHandler::RequestCompletionHandler(FRT_RPCRequest *parent)
+    : _parentRequest(parent)
+{
+}
+
+RequestCompletionHandler::~RequestCompletionHandler() {
+    if (_parentRequest) {
+        _parentRequest->SetError(FRTE_RPC_METHOD_FAILED, "removed before completion");
+        _parentRequest->Return();
+    }
+}
+
+void RequestCompletionHandler::doneHandler(OkState result) {
+    if (auto req = _parentRequest) {
+        _parentRequest = nullptr;
+        if (result.failed()) {
+            req->SetError(FRTE_RPC_METHOD_FAILED, result.errorMsg.c_str());
+        }
+        req->Return();
+    }
+}
+
+}
diff --git a/slobrok/src/vespa/slobrok/server/request_completion_handler.h b/slobrok/src/vespa/slobrok/server/request_completion_handler.h
new file mode 100644
index 00000000000..7b5d3104e54
--- /dev/null
+++ b/slobrok/src/vespa/slobrok/server/request_completion_handler.h
@@ -0,0 +1,28 @@
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+
+#pragma once
+
+#include "ok_state.h"
+
+class FRT_RPCRequest;
+
+namespace slobrok {
+
+/**
+ * Interface used to signal the result of LocalRpcMonitorMap::addLocal()
+ **/
+struct CompletionHandler {
+    virtual void doneHandler(OkState result) = 0;
+    virtual ~CompletionHandler() {}
+};
+
+class RequestCompletionHandler : public CompletionHandler {
+private:
+    FRT_RPCRequest *_parentRequest;
+public:
+    RequestCompletionHandler(FRT_RPCRequest *parentRequest);
+    virtual ~RequestCompletionHandler();
+    void doneHandler(OkState result) override;
+};
+
+}
diff --git a/slobrok/src/vespa/slobrok/server/rpchooks.cpp b/slobrok/src/vespa/slobrok/server/rpchooks.cpp
index ab1d5246ddc..b5e973e7c61 100644
--- a/slobrok/src/vespa/slobrok/server/rpchooks.cpp
+++ b/slobrok/src/vespa/slobrok/server/rpchooks.cpp
@@ -3,6 +3,7 @@
 #include "rpchooks.h"
 #include "ok_state.h"
 #include "named_service.h"
+#include "request_completion_handler.h"
 #include "rpc_server_map.h"
 #include "rpc_server_manager.h"
 #include "remote_slobrok.h"
@@ -37,12 +38,6 @@ public:
     ~MetricsReport() override { Kill(); }
 };
 
-struct ScriptCommandWrapper : LocalRpcMonitorMap::AddLocalCompletionHandler {
-    ScriptCommand script;
-    ScriptCommandWrapper(ScriptCommand &&script_in) : script(std::move(script_in)) {}
-    void doneHandler(OkState result) override { script.doneHandler(result); } 
-};
-
 } // namespace <unnamed>
 
 //-----------------------------------------------------------------------------
@@ -249,9 +244,8 @@ RPCHooks::rpc_registerRpcServer(FRT_RPCRequest *req)
     _cnts.registerReqs++;
     {
         // TODO: run only this path, and complete the request instead of ignoring
-        auto script = ScriptCommand::makeIgnoreCmd(_env, dName, dSpec);
         ServiceMapping mapping{dName, dSpec};
-        _env.localMonitorMap().addLocal(mapping, std::make_unique<ScriptCommandWrapper>(std::move(script)));
+        _env.localMonitorMap().addLocal(mapping, std::make_unique<RequestCompletionHandler>(nullptr));
     }
     // is this already OK?
     if (_rpcsrvmanager.alreadyManaged(dName, dSpec)) {
@@ -286,11 +280,10 @@ void RPCHooks::new_registerRpcServer(FRT_RPCRequest *req) {
         LOG(info, "cannot register %s at %s: conflict", dName, dSpec);
         return;
     }
-    auto script = ScriptCommand::makeRegCompleter(_env, dName, dSpec, req);
     req->Detach();
-    _env.localMonitorMap().addLocal(mapping, std::make_unique<ScriptCommandWrapper>(std::move(script)));
+    _env.localMonitorMap().addLocal(mapping, std::make_unique<RequestCompletionHandler>(req));
     // TODO: remove this
-    script = ScriptCommand::makeRegRpcSrvCmd(_env, dName, dSpec, nullptr);
+    auto script = ScriptCommand::makeRegRpcSrvCmd(_env, dName, dSpec, nullptr);
     script.doRequest();
     return;
 }