From 6cb063ee17cd67133fa1e7bf7b1e7ec3c08eb961 Mon Sep 17 00:00:00 2001 From: Henning Baldersheim Date: Fri, 28 Sep 2018 00:37:08 +0200 Subject: Use unique_ptr to signal ownership correctly --- .../src/vespa/slobrok/server/remote_slobrok.cpp | 2 +- .../vespa/slobrok/server/rpc_server_manager.cpp | 61 +++++++++++----------- .../src/vespa/slobrok/server/rpc_server_map.cpp | 16 +++--- slobrok/src/vespa/slobrok/server/rpc_server_map.h | 9 ++-- slobrok/src/vespa/slobrok/server/rpchooks.cpp | 2 +- slobrok/src/vespa/slobrok/server/selfcheck.cpp | 1 - slobrok/src/vespa/slobrok/server/visible_map.cpp | 14 ++--- slobrok/src/vespa/slobrok/server/visible_map.h | 10 ++-- 8 files changed, 56 insertions(+), 59 deletions(-) diff --git a/slobrok/src/vespa/slobrok/server/remote_slobrok.cpp b/slobrok/src/vespa/slobrok/server/remote_slobrok.cpp index 345a96ec9f1..d80faa695ca 100644 --- a/slobrok/src/vespa/slobrok/server/remote_slobrok.cpp +++ b/slobrok/src/vespa/slobrok/server/remote_slobrok.cpp @@ -79,7 +79,7 @@ RemoteSlobrok::doPending() NamedService *todo = _pending.front(); _pending.pop_front(); - NamedService *rpcsrv = _exchanger._rpcsrvmap.lookup(todo->getName()); + const NamedService *rpcsrv = _exchanger._rpcsrvmap.lookup(todo->getName()); if (rpcsrv == nullptr) { _remRemReq = getSupervisor()->AllocRPCRequest(); diff --git a/slobrok/src/vespa/slobrok/server/rpc_server_manager.cpp b/slobrok/src/vespa/slobrok/server/rpc_server_manager.cpp index e18e13d8d59..ba7d94e3885 100644 --- a/slobrok/src/vespa/slobrok/server/rpc_server_manager.cpp +++ b/slobrok/src/vespa/slobrok/server/rpc_server_manager.cpp @@ -54,7 +54,7 @@ RpcServerManager::checkPartner(const std::string & remslobrok) if (remslobrok == _env.mySpec()) { return OkState(13, "remote slobrok using my rpcserver name"); } - RemoteSlobrok *partner = _exchanger.lookupPartner(remslobrok); + const RemoteSlobrok *partner = _exchanger.lookupPartner(remslobrok); if (partner == nullptr) { return OkState(13, "remote slobrok not a partner"); } @@ -70,7 +70,7 @@ RpcServerManager::addRemReservation(const std::string & remslobrok, const std::s OkState valid = validateName(name); if (valid.failed()) return valid; - NamedService *old = _rpcsrvmap.lookupManaged(name); + const NamedService *old = _rpcsrvmap.lookupManaged(name); if (old != nullptr) { if (old->getSpec() == spec) { // was alright already @@ -83,7 +83,7 @@ RpcServerManager::addRemReservation(const std::string & remslobrok, const std::s if (_rpcsrvmap.conflictingReservation(name, spec)) { return OkState(FRTE_RPC_METHOD_FAILED, "registration for name already in progress"); } - _rpcsrvmap.addReservation(new ReservedName(name, spec, false)); + _rpcsrvmap.addReservation(std::make_unique(name, spec, false)); return OkState(0, "done"); } @@ -104,7 +104,7 @@ RpcServerManager::removePeer(const std::string & remsbname, const std::string & if (remsbname == _env.mySpec()) { return OkState(13, "cannot remove my own rpcserver name"); } - RemoteSlobrok *partner = _exchanger.lookupPartner(remsbname); + const RemoteSlobrok *partner = _exchanger.lookupPartner(remsbname); if (partner == nullptr) { return OkState(0, "remote slobrok not a partner"); } @@ -122,7 +122,7 @@ RpcServerManager::addMyReservation(const std::string & name, const std::string & OkState valid = validateName(name); if (valid.failed()) return valid; - NamedService *old = _rpcsrvmap.lookupManaged(name); + const NamedService *old = _rpcsrvmap.lookupManaged(name); if (old != nullptr) { if (old->getSpec() == spec) { // was alright already @@ -143,8 +143,7 @@ RpcServerManager::addMyReservation(const std::string & name, const std::string & "registration for name already in progress with a different spec"); } _rpcsrvmap.removeReservation(name); - ReservedName *rpcsrv = new ReservedName(name, spec, true); - _rpcsrvmap.addReservation(rpcsrv); + _rpcsrvmap.addReservation(std::make_unique(name, spec, true)); return OkState(0, "done"); } @@ -158,7 +157,7 @@ RpcServerManager::addRemote(const std::string & name, const std::string &spec) if (alreadyManaged(name, spec)) { return OkState(0, "already correct"); } - NamedService *old = _rpcsrvmap.lookup(name); + const NamedService *old = _rpcsrvmap.lookup(name); if (old != nullptr) { if (old->getSpec() != spec) { LOG(warning, "collision on remote add: name %s registered to %s locally, " @@ -172,16 +171,17 @@ RpcServerManager::addRemote(const std::string & name, const std::string &spec) return OkState(0, "already correct"); } _rpcsrvmap.removeReservation(name); - ManagedRpcServer *rpcsrv = new ManagedRpcServer(name, spec, *this); - _rpcsrvmap.addNew(rpcsrv); - rpcsrv->healthCheck(); + auto rpcsrv = std::make_unique(name, spec, *this); + ManagedRpcServer & rpcServer = *rpcsrv; + _rpcsrvmap.addNew(std::move(rpcsrv)); + rpcServer.healthCheck(); return OkState(0, "done"); } OkState RpcServerManager::remove(ManagedRpcServer *rpcsrv) { - NamedService *td = _rpcsrvmap.lookup(rpcsrv->getName()); + const NamedService *td = _rpcsrvmap.lookup(rpcsrv->getName()); if (td == rpcsrv) { return removeLocal(rpcsrv->getName(), rpcsrv->getSpec()); } else { @@ -193,7 +193,7 @@ RpcServerManager::remove(ManagedRpcServer *rpcsrv) OkState RpcServerManager::removeRemote(const std::string &name, const std::string &spec) { - NamedService *old = _rpcsrvmap.lookup(name); + const NamedService *old = _rpcsrvmap.lookup(name); if (old == nullptr) { // was alright already, remove any reservation too _rpcsrvmap.removeReservation(name); @@ -202,27 +202,26 @@ RpcServerManager::removeRemote(const std::string &name, const std::string &spec) if (old->getSpec() != spec) { return OkState(1, "name registered, but with different spec"); } - NamedService *td = _rpcsrvmap.remove(name); - LOG_ASSERT(td == old); - delete td; + std::unique_ptr td = _rpcsrvmap.remove(name); + LOG_ASSERT(td.get() == old); return OkState(0, "done"); } OkState RpcServerManager::removeLocal(const std::string & name, const std::string &spec) { - NamedService *td = _rpcsrvmap.lookup(name); + const NamedService *td = _rpcsrvmap.lookup(name); if (td == nullptr) { // already removed, nop return OkState(); } - RemoteSlobrok *partner = _exchanger.lookupPartner(name); + const RemoteSlobrok *partner = _exchanger.lookupPartner(name); if (partner != nullptr) { return OkState(13, "cannot unregister partner slobrok"); } - ManagedRpcServer *rpcsrv = _rpcsrvmap.lookupManaged(name); + const ManagedRpcServer *rpcsrv = _rpcsrvmap.lookupManaged(name); if (rpcsrv == nullptr) { return OkState(13, "not a local rpcserver"); } @@ -232,9 +231,8 @@ RpcServerManager::removeLocal(const std::string & name, const std::string &spec) // or log it on level INFO? return OkState(1, "name registered, but with different spec"); } - td = _rpcsrvmap.remove(name); - LOG_ASSERT(td == rpcsrv); - delete rpcsrv; + auto tdUP = _rpcsrvmap.remove(name); + LOG_ASSERT(tdUP.get() == rpcsrv); _exchanger.forwardRemove(name, spec); return OkState(); } @@ -243,19 +241,20 @@ RpcServerManager::removeLocal(const std::string & name, const std::string &spec) void RpcServerManager::addManaged(const std::string &name, const std::string &spec, RegRpcSrvCommand rdc) { - ManagedRpcServer *rpcsrv = new ManagedRpcServer(name, spec, *this); - _rpcsrvmap.addNew(rpcsrv); + auto newRpcServer = std::make_unique(name, spec, *this); + ManagedRpcServer & rpcsrv = *newRpcServer; + _rpcsrvmap.addNew(std::move(newRpcServer)); for (size_t i = 0; i < _addManageds.size(); i++) { if (_addManageds[i].rpcsrv == nullptr) { - _addManageds[i].rpcsrv = rpcsrv; + _addManageds[i].rpcsrv = &rpcsrv; _addManageds[i].handler = rdc; - rpcsrv->healthCheck(); + rpcsrv.healthCheck(); return; } } - MRSandRRSC pair(rpcsrv, rdc); + MRSandRRSC pair(&rpcsrv, rdc); _addManageds.push_back(pair); - rpcsrv->healthCheck(); + rpcsrv.healthCheck(); return; } @@ -264,7 +263,7 @@ RpcServerManager::addManaged(const std::string &name, const std::string &spec, R bool RpcServerManager::alreadyManaged(const std::string &name, const std::string &spec) { - ManagedRpcServer *rpcsrv = _rpcsrvmap.lookupManaged(name); + const ManagedRpcServer *rpcsrv = _rpcsrvmap.lookupManaged(name); if (rpcsrv != nullptr) { if (rpcsrv->getSpec() == spec) { return true; @@ -297,9 +296,9 @@ RpcServerManager::notifyFailedRpcSrv(ManagedRpcServer *rpcsrv, std::string errms { _env.countFailedHeartbeat(); bool logged = false; - NamedService *old = _rpcsrvmap.lookup(rpcsrv->getName()); + const NamedService *old = _rpcsrvmap.lookup(rpcsrv->getName()); if (old == rpcsrv) { - old = _rpcsrvmap.remove(rpcsrv->getName()); + old = _rpcsrvmap.remove(rpcsrv->getName()).release(); LOG_ASSERT(old == rpcsrv); LOG(info, "managed server %s at %s failed: %s", rpcsrv->getName().c_str(), rpcsrv->getSpec().c_str(), errmsg.c_str()); diff --git a/slobrok/src/vespa/slobrok/server/rpc_server_map.cpp b/slobrok/src/vespa/slobrok/server/rpc_server_map.cpp index 34f8f7b9135..e19a91cd78d 100644 --- a/slobrok/src/vespa/slobrok/server/rpc_server_map.cpp +++ b/slobrok/src/vespa/slobrok/server/rpc_server_map.cpp @@ -18,19 +18,19 @@ RpcServerMap::lookupManaged(const std::string & name) const { return (found == _myrpcsrv_map.end()) ? nullptr : found->second.get(); } -NamedService * +const NamedService * RpcServerMap::lookup(const std::string & name) const { return lookupManaged(name); } -NamedService * +std::unique_ptr RpcServerMap::remove(const std::string & name) { _visible_map.remove(name); auto service = std::move(_myrpcsrv_map[name]); _myrpcsrv_map.erase(name); - return service.release(); + return service; } std::vector @@ -73,7 +73,7 @@ RpcServerMap::add(NamedService *rpcsrv) } void -RpcServerMap::addNew(ManagedRpcServer *rpcsrv) +RpcServerMap::addNew(std::unique_ptr rpcsrv) { const std::string &name = rpcsrv->getName(); @@ -102,13 +102,13 @@ RpcServerMap::addNew(ManagedRpcServer *rpcsrv) } } } - add(rpcsrv); - _myrpcsrv_map[name].reset(rpcsrv); + add(rpcsrv.get()); + _myrpcsrv_map[name] = std::move(rpcsrv); } void -RpcServerMap::addReservation(ReservedName *rpcsrv) +RpcServerMap::addReservation(std::unique_ptr rpcsrv) { LOG_ASSERT(rpcsrv != nullptr); LOG_ASSERT(_myrpcsrv_map.find(rpcsrv->getName()) == _myrpcsrv_map.end()); @@ -117,10 +117,10 @@ RpcServerMap::addReservation(ReservedName *rpcsrv) // this should have been checked already, so assert LOG_ASSERT(! conflictingReservation(rpcsrv->getName(), rpcsrv->getSpec())); auto old = std::move(_reservations[rpcsrv->getName()]); - _reservations[rpcsrv->getName()].reset(rpcsrv); LOG_ASSERT(!old || old->getSpec() == rpcsrv->getSpec() || ! old->stillReserved()); + _reservations[rpcsrv->getName()] = std::move(rpcsrv); } diff --git a/slobrok/src/vespa/slobrok/server/rpc_server_map.h b/slobrok/src/vespa/slobrok/server/rpc_server_map.h index d458df6cb1b..2c69c1b8cde 100644 --- a/slobrok/src/vespa/slobrok/server/rpc_server_map.h +++ b/slobrok/src/vespa/slobrok/server/rpc_server_map.h @@ -38,15 +38,14 @@ public: ManagedRpcServer *lookupManaged(const std::string & name) const; - NamedService * lookup(const std::string & name) const; + const NamedService * lookup(const std::string & name) const; RpcSrvlist lookupPattern(const char *pattern) const; - RpcSrvlist allVisible() const; RpcSrvlist allManaged() const; - void addNew(ManagedRpcServer *rpcsrv); - NamedService * remove(const std::string & name); + void addNew(std::unique_ptr rpcsrv); + std::unique_ptr remove(const std::string & name); - void addReservation(ReservedName *rpcsrv); + void addReservation(std::unique_ptrrpcsrv); bool conflictingReservation(const std::string & name, const std::string &spec); const ReservedName *getReservation(const std::string & name) const; diff --git a/slobrok/src/vespa/slobrok/server/rpchooks.cpp b/slobrok/src/vespa/slobrok/server/rpchooks.cpp index 133de63c895..9abbf6ab3fe 100644 --- a/slobrok/src/vespa/slobrok/server/rpchooks.cpp +++ b/slobrok/src/vespa/slobrok/server/rpchooks.cpp @@ -466,7 +466,7 @@ RPCHooks::rpc_lookupManaged(FRT_RPCRequest *req) FRT_Values &args = *req->GetParams(); const char *name = args[0]._string._str; LOG(debug, "RPC: lookupManaged(%s)", name); - ManagedRpcServer *found = _rpcsrvmap.lookupManaged(name); + const ManagedRpcServer *found = _rpcsrvmap.lookupManaged(name); if (found == nullptr) { req->SetError(FRTE_RPC_METHOD_FAILED, "Not found"); diff --git a/slobrok/src/vespa/slobrok/server/selfcheck.cpp b/slobrok/src/vespa/slobrok/server/selfcheck.cpp index a5e6870882e..7fc639ec028 100644 --- a/slobrok/src/vespa/slobrok/server/selfcheck.cpp +++ b/slobrok/src/vespa/slobrok/server/selfcheck.cpp @@ -51,5 +51,4 @@ SelfCheck::PerformTask() Schedule(seconds); } - } // namespace slobrok diff --git a/slobrok/src/vespa/slobrok/server/visible_map.cpp b/slobrok/src/vespa/slobrok/server/visible_map.cpp index a9257dbb391..d26bb49d874 100644 --- a/slobrok/src/vespa/slobrok/server/visible_map.cpp +++ b/slobrok/src/vespa/slobrok/server/visible_map.cpp @@ -54,7 +54,7 @@ VisibleMap::removeUpdateListener(IUpdateListener *l) //----------------------------------------------------------------------------- -NamedService * +const NamedService * VisibleMap::lookup(const std::string &name) const { auto found = _map.find(name); return (found == _map.end()) ? nullptr : found->second; @@ -74,7 +74,7 @@ VisibleMap::allVisible() const void -VisibleMap::addNew(NamedService *rpcsrv) +VisibleMap::addNew(const NamedService *rpcsrv) { LOG_ASSERT(rpcsrv != nullptr); LOG_ASSERT(_map.find(rpcsrv->getName()) == _map.end()); @@ -85,10 +85,10 @@ VisibleMap::addNew(NamedService *rpcsrv) } -NamedService * +const NamedService * VisibleMap::remove(const std::string &name) { - NamedService *d = _map[name]; + const NamedService *d = _map[name]; _map.erase(name); if (d != nullptr) { _history.add(name, _genCnt); @@ -98,11 +98,11 @@ VisibleMap::remove(const std::string &name) { } -NamedService * -VisibleMap::update(NamedService *rpcsrv) { +const NamedService * +VisibleMap::update(const NamedService *rpcsrv) { LOG_ASSERT(rpcsrv != nullptr); - NamedService *d = rpcsrv; + const NamedService *d = rpcsrv; std::swap(d, _map[rpcsrv->getName()]); LOG_ASSERT(d != nullptr); diff --git a/slobrok/src/vespa/slobrok/server/visible_map.h b/slobrok/src/vespa/slobrok/server/visible_map.h index 5d2440cd176..6ea2d5e2a46 100644 --- a/slobrok/src/vespa/slobrok/server/visible_map.h +++ b/slobrok/src/vespa/slobrok/server/visible_map.h @@ -45,7 +45,7 @@ public: }; private: - using Map = std::unordered_map; + using Map = std::unordered_map; using WaitList = std::vector; Map _map; @@ -62,11 +62,11 @@ public: void addUpdateListener(IUpdateListener *l); void removeUpdateListener(IUpdateListener *l); - void addNew(NamedService *rpcsrv); - NamedService *remove(const std::string &name); - NamedService *update(NamedService *rpcsrv); + void addNew(const NamedService *rpcsrv); + const NamedService *remove(const std::string &name); + const NamedService *update(const NamedService *rpcsrv); - NamedService *lookup(const std::string &name) const; + const NamedService *lookup(const std::string &name) const; RpcSrvlist allVisible() const; const vespalib::GenCnt& genCnt() { return _genCnt; } -- cgit v1.2.3