From 79de867efd5d98c5eabe37322c6caada3dd44a8a Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Thu, 10 Jul 2025 17:04:21 -0400 Subject: [PATCH 1/5] revert calling private setter --- src/viam/sdk/spatialmath/orientation.cpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/viam/sdk/spatialmath/orientation.cpp b/src/viam/sdk/spatialmath/orientation.cpp index f87c17a36..2079da80c 100644 --- a/src/viam/sdk/spatialmath/orientation.cpp +++ b/src/viam/sdk/spatialmath/orientation.cpp @@ -51,7 +51,6 @@ void to_proto_impl::operator()(const Orientation& self, aa.set_z(a.z); aa.set_theta(a.theta); *orientation.mutable_axis_angles() = std::move(aa); - orientation.set_has_axis_angles(); } void operator()(const orientation_vector& ov) { @@ -61,7 +60,6 @@ void to_proto_impl::operator()(const Orientation& self, ovec.set_z(ov.z); ovec.set_theta(ov.theta); *orientation.mutable_vector_radians() = std::move(ovec); - orientation.set_has_vector_radians(); } void operator()(const orientation_vector_degrees& ovd) { @@ -71,7 +69,6 @@ void to_proto_impl::operator()(const Orientation& self, ovec.set_z(ovd.z); ovec.set_theta(ovd.theta); *orientation.mutable_vector_degrees() = std::move(ovec); - orientation.set_has_vector_degrees(); } void operator()(const euler_angles& ea) { @@ -80,7 +77,6 @@ void to_proto_impl::operator()(const Orientation& self, euler.set_roll(ea.roll); euler.set_yaw(ea.yaw); *orientation.mutable_euler_angles() = std::move(euler); - orientation.set_has_euler_angles(); } void operator()(const quaternion& q) { @@ -90,7 +86,6 @@ void to_proto_impl::operator()(const Orientation& self, quat.set_y(q.y); quat.set_z(q.z); *orientation.mutable_quaternion() = std::move(quat); - orientation.set_has_quaternion(); } app::v1::Orientation& orientation; From 02d422f95c2a5988c0a8f9f1ce91d3d3da405d5c Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Fri, 11 Jul 2025 16:31:04 -0400 Subject: [PATCH 2/5] make a version of replace_one that deletes before constructing --- src/viam/sdk/module/service.cpp | 40 +++++++++++++--------- src/viam/sdk/resource/resource_manager.cpp | 12 +++++++ src/viam/sdk/resource/resource_manager.hpp | 7 ++++ 3 files changed, 42 insertions(+), 17 deletions(-) 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..18088a934 100644 --- a/src/viam/sdk/resource/resource_manager.cpp +++ b/src/viam/sdk/resource/resource_manager.cpp @@ -150,6 +150,18 @@ void ResourceManager::replace_one(const Name& name, std::shared_ptr re } } +void ResourceManager::replace_one(const Name& name, + std::function()> create_resource) { + const std::lock_guard lock(lock_); + try { + do_remove(name); + do_add(name, create_resource()); + } catch (std::exception& exc) { + VIAM_SDK_LOG(error) << "failed to replace resource " << name.to_string() << ": " + << exc.what(); + } +} + const std::unordered_map>& ResourceManager::resources() const { return resources_; diff --git a/src/viam/sdk/resource/resource_manager.hpp b/src/viam/sdk/resource/resource_manager.hpp index 0d0c27ee8..ec2e75d95 100644 --- a/src/viam/sdk/resource/resource_manager.hpp +++ b/src/viam/sdk/resource/resource_manager.hpp @@ -58,8 +58,15 @@ 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. + [[deprecated("Use the callback overload as it destroys before constructing")]] void replace_one(const Name& name, std::shared_ptr resource); + /// @brief Replaces an existing resource. No-op if the named resource does not exist. + /// @param name The name of the resource to replace. + /// @param create_resource Callback to construct the new resource that is replacing the existing + /// one. + void replace_one(const Name& name, std::function()> create_resource); + /// @brief Returns a reference to the existing resources within the manager. const std::unordered_map>& resources() const; From 47b9b087b710b53a9d66ed1b6b0925852418e6f5 Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Mon, 14 Jul 2025 09:14:45 -0400 Subject: [PATCH 3/5] silence linter --- src/viam/sdk/resource/resource_manager.cpp | 4 ++-- src/viam/sdk/resource/resource_manager.hpp | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/viam/sdk/resource/resource_manager.cpp b/src/viam/sdk/resource/resource_manager.cpp index 18088a934..852b1c7ac 100644 --- a/src/viam/sdk/resource/resource_manager.cpp +++ b/src/viam/sdk/resource/resource_manager.cpp @@ -150,8 +150,8 @@ void ResourceManager::replace_one(const Name& name, std::shared_ptr re } } -void ResourceManager::replace_one(const Name& name, - std::function()> create_resource) { +void ResourceManager::replace_one( + const Name& name, const std::function()>& create_resource) { const std::lock_guard lock(lock_); try { do_remove(name); diff --git a/src/viam/sdk/resource/resource_manager.hpp b/src/viam/sdk/resource/resource_manager.hpp index ec2e75d95..f77582506 100644 --- a/src/viam/sdk/resource/resource_manager.hpp +++ b/src/viam/sdk/resource/resource_manager.hpp @@ -65,7 +65,8 @@ class ResourceManager { /// @param name The name of the resource to replace. /// @param create_resource Callback to construct the new resource that is replacing the existing /// one. - void replace_one(const Name& name, std::function()> create_resource); + 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; From 1c287dbbf24a2fbb0e81d4c149b429edb75fcb9b Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Mon, 14 Jul 2025 09:15:56 -0400 Subject: [PATCH 4/5] silence formatter --- src/viam/sdk/resource/resource_manager.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/viam/sdk/resource/resource_manager.hpp b/src/viam/sdk/resource/resource_manager.hpp index f77582506..4b19aeb95 100644 --- a/src/viam/sdk/resource/resource_manager.hpp +++ b/src/viam/sdk/resource/resource_manager.hpp @@ -58,8 +58,8 @@ 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. - [[deprecated("Use the callback overload as it destroys before constructing")]] - void replace_one(const Name& name, std::shared_ptr resource); + [[deprecated("Use the callback overload as it destroys before constructing")]] void replace_one( + const Name& name, std::shared_ptr resource); /// @brief Replaces an existing resource. No-op if the named resource does not exist. /// @param name The name of the resource to replace. From ea9b4caa09b0c52a8ef94c836ccdf2ff38469420 Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Mon, 14 Jul 2025 11:33:24 -0400 Subject: [PATCH 5/5] remove broken version --- src/viam/sdk/resource/resource_manager.cpp | 11 ----------- src/viam/sdk/resource/resource_manager.hpp | 6 ------ 2 files changed, 17 deletions(-) diff --git a/src/viam/sdk/resource/resource_manager.cpp b/src/viam/sdk/resource/resource_manager.cpp index 852b1c7ac..d545d26ef 100644 --- a/src/viam/sdk/resource/resource_manager.cpp +++ b/src/viam/sdk/resource/resource_manager.cpp @@ -139,17 +139,6 @@ void ResourceManager::remove(const Name& name) { } } -void ResourceManager::replace_one(const Name& name, std::shared_ptr resource) { - const std::lock_guard lock(lock_); - try { - do_remove(name); - do_add(name, std::move(resource)); - } catch (std::exception& exc) { - VIAM_SDK_LOG(error) << "failed to replace resource " << name.to_string() << ": " - << exc.what(); - } -} - void ResourceManager::replace_one( const Name& name, const std::function()>& create_resource) { const std::lock_guard lock(lock_); diff --git a/src/viam/sdk/resource/resource_manager.hpp b/src/viam/sdk/resource/resource_manager.hpp index 4b19aeb95..b59b7310a 100644 --- a/src/viam/sdk/resource/resource_manager.hpp +++ b/src/viam/sdk/resource/resource_manager.hpp @@ -55,12 +55,6 @@ class ResourceManager { /// @param name The name of the resource to remove. void remove(const Name& name); - /// @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. - [[deprecated("Use the callback overload as it destroys before constructing")]] void replace_one( - const Name& name, std::shared_ptr resource); - /// @brief Replaces an existing resource. No-op if the named resource does not exist. /// @param name The name of the resource to replace. /// @param create_resource Callback to construct the new resource that is replacing the existing