diff --git a/src/viam/sdk/module/service.cpp b/src/viam/sdk/module/service.cpp index 530165b84..26ec3c84b 100644 --- a/src/viam/sdk/module/service.cpp +++ b/src/viam/sdk/module/service.cpp @@ -110,23 +110,28 @@ struct ModuleService::ServiceImpl : viam::module::v1::ModuleService::Service { } auto manager = resource_server->resource_manager(); - // see if our resource is reconfigurable. if it is, reconfigure - const std::shared_ptr res = manager->resource(cfg.resource_name().name()); - if (!res) { - return grpc::Status(grpc::UNKNOWN, - "unable to reconfigure resource " + cfg.resource_name().name() + - " as it doesn't exist."); - } + // Explicitly open a scope to control the lifetime of the shared_ptr, otherwise + // `res` below will keep the refcount high until the function exits, fouling up the order of + // operations for replacing a resource. + { + // see if our resource is reconfigurable. if it is, reconfigure + const std::shared_ptr res = manager->resource(cfg.resource_name().name()); + if (!res) { + return grpc::Status(grpc::UNKNOWN, + "unable to reconfigure resource " + cfg.resource_name().name() + + " as it doesn't exist."); + } - if (auto reconfigurable = std::dynamic_pointer_cast(res)) { - reconfigurable->reconfigure(deps, cfg); - res->set_log_level(cfg.get_log_level()); - return grpc::Status(); - } + if (auto reconfigurable = std::dynamic_pointer_cast(res)) { + reconfigurable->reconfigure(deps, cfg); + res->set_log_level(cfg.get_log_level()); + return grpc::Status(); + } - // if the type isn't reconfigurable by default, replace it - if (auto stoppable = std::dynamic_pointer_cast(res)) { - stoppable->stop(); + // if the type isn't reconfigurable by default, replace it + if (auto stoppable = std::dynamic_pointer_cast(res)) { + stoppable->stop(); + } } const std::shared_ptr reg = @@ -138,8 +143,9 @@ struct ModuleService::ServiceImpl : viam::module::v1::ModuleService::Service { } try { - std::shared_ptr resource = reg->construct_resource(deps, cfg); - manager->replace_one(cfg.resource_name(), std::move(resource)); + manager->replace_one(cfg.resource_name(), [®, &deps, &cfg]() { + return reg->construct_resource(deps, cfg); + }); } catch (const std::exception& exc) { return grpc::Status(::grpc::INTERNAL, exc.what()); } diff --git a/src/viam/sdk/resource/resource_manager.cpp b/src/viam/sdk/resource/resource_manager.cpp index 3c62faae7..d545d26ef 100644 --- a/src/viam/sdk/resource/resource_manager.cpp +++ b/src/viam/sdk/resource/resource_manager.cpp @@ -139,11 +139,12 @@ void ResourceManager::remove(const Name& name) { } } -void ResourceManager::replace_one(const Name& name, std::shared_ptr resource) { +void ResourceManager::replace_one( + const Name& name, const std::function()>& create_resource) { const std::lock_guard lock(lock_); try { do_remove(name); - do_add(name, std::move(resource)); + do_add(name, create_resource()); } catch (std::exception& exc) { VIAM_SDK_LOG(error) << "failed to replace resource " << name.to_string() << ": " << exc.what(); diff --git a/src/viam/sdk/resource/resource_manager.hpp b/src/viam/sdk/resource/resource_manager.hpp index 0d0c27ee8..b59b7310a 100644 --- a/src/viam/sdk/resource/resource_manager.hpp +++ b/src/viam/sdk/resource/resource_manager.hpp @@ -57,8 +57,10 @@ class ResourceManager { /// @brief Replaces an existing resource. No-op if the named resource does not exist. /// @param name The name of the resource to replace. - /// @param resource The new resource that is replacing the existing one. - void replace_one(const Name& name, std::shared_ptr resource); + /// @param create_resource Callback to construct the new resource that is replacing the existing + /// one. + void replace_one(const Name& name, + const std::function()>& create_resource); /// @brief Returns a reference to the existing resources within the manager. const std::unordered_map>& resources() const;