From e003d82d368a260abc33a406c741a04faacf5215 Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Tue, 7 Jan 2025 11:22:20 -0500 Subject: [PATCH 1/5] refer to grpc status by pointer in grpc exception --- src/viam/sdk/common/client_helper.cpp | 9 +++++-- src/viam/sdk/common/client_helper.hpp | 25 ++++++++----------- src/viam/sdk/common/exception.cpp | 17 ++++++++----- src/viam/sdk/common/exception.hpp | 17 +++++++++---- .../sdk/components/private/board_client.cpp | 4 +-- .../sdk/services/private/mlmodel_client.cpp | 2 +- 6 files changed, 44 insertions(+), 30 deletions(-) diff --git a/src/viam/sdk/common/client_helper.cpp b/src/viam/sdk/common/client_helper.cpp index 1d98249b6..f59d56d9f 100644 --- a/src/viam/sdk/common/client_helper.cpp +++ b/src/viam/sdk/common/client_helper.cpp @@ -3,6 +3,7 @@ #include #include +#include #include @@ -13,12 +14,16 @@ namespace sdk { namespace client_helper_details { -[[noreturn]] void errorHandlerReturnedUnexpectedly(const ::grpc::Status& status) noexcept { +[[noreturn]] void errorHandlerReturnedUnexpectedly(const ::grpc::Status* status) noexcept { BOOST_LOG_TRIVIAL(fatal) << "ClientHelper error handler callback returned instead of throwing: " - << status.error_message() << '(' << status.error_details() << ')'; + << status->error_message() << '(' << status->error_details() << ')'; std::abort(); } +bool isStatusCancelled(int status) noexcept { + return status = ::grpc::StatusCode::CANCELLED; +} + } // namespace client_helper_details ClientContext::ClientContext() : wrapped_context_(std::make_unique()) { diff --git a/src/viam/sdk/common/client_helper.hpp b/src/viam/sdk/common/client_helper.hpp index f4fb5406a..d572307b7 100644 --- a/src/viam/sdk/common/client_helper.hpp +++ b/src/viam/sdk/common/client_helper.hpp @@ -5,18 +5,15 @@ #include #include -namespace grpc { - -class Status; - -} // namespace grpc - namespace viam { namespace sdk { namespace client_helper_details { -[[noreturn]] void errorHandlerReturnedUnexpectedly(const ::grpc::Status&) noexcept; +[[noreturn]] void errorHandlerReturnedUnexpectedly(const ::grpc::Status*) noexcept; + +// Helper function to test equality of status with grpc::StatusCode::CANCELLED. +bool isStatusCancelled(int status) noexcept; } // namespace client_helper_details @@ -62,7 +59,7 @@ template (response_)); } - std::forward(ehc)(result); - client_helper_details::errorHandlerReturnedUnexpectedly(result); + std::forward(ehc)(&result); + client_helper_details::errorHandlerReturnedUnexpectedly(&result); } // A version of invoke for gRPC calls returning `(stream ResponseType)`. @@ -138,13 +135,13 @@ class ClientHelper { const auto result = reader->Finish(); - if (result.ok() || - (cancelled_by_handler && result.error_code() == ::grpc::StatusCode::CANCELLED)) { + if (result.ok() || (cancelled_by_handler && + client_helper_details::isStatusCancelled(result.error_code()))) { return; } - std::forward(ehc)(result); - client_helper_details::errorHandlerReturnedUnexpectedly(result); + std::forward(ehc)(&result); + client_helper_details::errorHandlerReturnedUnexpectedly(&result); } private: diff --git a/src/viam/sdk/common/exception.cpp b/src/viam/sdk/common/exception.cpp index f9832e122..3a76efde4 100644 --- a/src/viam/sdk/common/exception.cpp +++ b/src/viam/sdk/common/exception.cpp @@ -1,12 +1,14 @@ #include +#include + namespace viam { namespace sdk { Exception::Exception(ErrorCondition condition, const std::string& what) - : std::runtime_error("viam::sdk::Exception: " + what), condition_(condition){}; + : std::runtime_error("viam::sdk::Exception: " + what), condition_(condition) {}; -Exception::Exception(const std::string& what) : Exception(ErrorCondition::k_general, what){}; +Exception::Exception(const std::string& what) : Exception(ErrorCondition::k_general, what) {}; Exception::~Exception() = default; @@ -45,11 +47,14 @@ std::error_condition make_error_condition(ErrorCondition e) { return {static_cast(e), errorCategory}; } -GRPCException::GRPCException(grpc::Status status) - : Exception(ErrorCondition::k_grpc, status.error_message()), status_(std::move(status)){}; +GRPCException::GRPCException(const grpc::Status* status) + : Exception(ErrorCondition::k_grpc, status->error_message()), + status_(std::make_unique(*status)) {} + +GRPCException::~GRPCException() = default; -const grpc::Status& GRPCException::status() const noexcept { - return status_; +const grpc::Status* GRPCException::status() const noexcept { + return status_.get(); } } // namespace sdk diff --git a/src/viam/sdk/common/exception.hpp b/src/viam/sdk/common/exception.hpp index c9ab11e3b..adfdc6ecd 100644 --- a/src/viam/sdk/common/exception.hpp +++ b/src/viam/sdk/common/exception.hpp @@ -2,11 +2,17 @@ /// /// @brief Defines custom exceptions for the SDK. #pragma once -#include + +#include #include #include +#include + +namespace grpc { + +class Status; -#include +} // namespace grpc namespace viam { namespace sdk { @@ -49,12 +55,13 @@ class Exception : public std::runtime_error { /// @ingroup Exception class GRPCException : public Exception { public: - explicit GRPCException(grpc::Status status); + explicit GRPCException(const grpc::Status* status); + ~GRPCException(); - const grpc::Status& status() const noexcept; + const grpc::Status* status() const noexcept; private: - grpc::Status status_; + std::unique_ptr status_; }; } // namespace sdk diff --git a/src/viam/sdk/components/private/board_client.cpp b/src/viam/sdk/components/private/board_client.cpp index 11af0428a..467be22fa 100644 --- a/src/viam/sdk/components/private/board_client.cpp +++ b/src/viam/sdk/components/private/board_client.cpp @@ -123,7 +123,7 @@ Board::analog_response BoardClient::read_analog(const std::string& analog_reader const grpc::Status status = stub_->ReadAnalogReader(ctx, request, &response); if (!status.ok()) { - throw GRPCException(status); + throw GRPCException(&status); } return Board::analog_response{ response.value(), response.min_range(), response.max_range(), response.step_size()}; @@ -153,7 +153,7 @@ Board::digital_value BoardClient::read_digital_interrupt(const std::string& digi const grpc::Status status = stub_->GetDigitalInterruptValue(ctx, request, &response); if (!status.ok()) { - throw GRPCException(status); + throw GRPCException(&status); } return response.value(); } diff --git a/src/viam/sdk/services/private/mlmodel_client.cpp b/src/viam/sdk/services/private/mlmodel_client.cpp index 2c98960dd..6792885df 100644 --- a/src/viam/sdk/services/private/mlmodel_client.cpp +++ b/src/viam/sdk/services/private/mlmodel_client.cpp @@ -71,7 +71,7 @@ std::shared_ptr MLModelServiceClient::infer( const auto result = stub_->Infer(ctx, *req, resp); if (!result.ok()) { - throw GRPCException(result); + throw GRPCException(&result); } for (const auto& kv : resp->output_tensors().tensors()) { From 546d78283c3d38f1b741d83f2ecdc59bb7310a0c Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Tue, 7 Jan 2025 12:30:01 -0500 Subject: [PATCH 2/5] make service_helper private --- src/viam/sdk/CMakeLists.txt | 3 +-- src/viam/sdk/common/{ => private}/service_helper.cpp | 2 +- src/viam/sdk/common/{ => private}/service_helper.hpp | 0 src/viam/sdk/components/private/arm_server.cpp | 2 +- src/viam/sdk/components/private/base_server.cpp | 2 +- src/viam/sdk/components/private/board_server.cpp | 2 +- src/viam/sdk/components/private/camera_server.cpp | 2 +- src/viam/sdk/components/private/encoder_server.cpp | 2 +- src/viam/sdk/components/private/gantry_server.cpp | 2 +- src/viam/sdk/components/private/generic_server.cpp | 2 +- src/viam/sdk/components/private/gripper_server.cpp | 2 +- src/viam/sdk/components/private/motor_server.cpp | 2 +- src/viam/sdk/components/private/movement_sensor_server.cpp | 2 +- src/viam/sdk/components/private/pose_tracker_server.cpp | 2 +- src/viam/sdk/components/private/power_sensor_server.cpp | 2 +- src/viam/sdk/components/private/sensor_server.cpp | 2 +- src/viam/sdk/components/private/servo_server.cpp | 2 +- src/viam/sdk/services/private/generic_server.cpp | 2 +- src/viam/sdk/services/private/mlmodel_server.cpp | 2 +- src/viam/sdk/services/private/motion_server.cpp | 2 +- src/viam/sdk/services/private/navigation_server.cpp | 2 +- 21 files changed, 20 insertions(+), 21 deletions(-) rename src/viam/sdk/common/{ => private}/service_helper.cpp (96%) rename src/viam/sdk/common/{ => private}/service_helper.hpp (100%) diff --git a/src/viam/sdk/CMakeLists.txt b/src/viam/sdk/CMakeLists.txt index ad2bfe793..4c9acc36b 100644 --- a/src/viam/sdk/CMakeLists.txt +++ b/src/viam/sdk/CMakeLists.txt @@ -53,10 +53,10 @@ target_sources(viamsdk common/linear_algebra.cpp common/pose.cpp common/proto_value.cpp - common/service_helper.cpp common/utils.cpp common/version_metadata.cpp common/world_state.cpp + common/private/service_helper.cpp components/arm.cpp components/base.cpp components/board.cpp @@ -147,7 +147,6 @@ target_sources(viamsdk ../../viam/sdk/common/pose.hpp ../../viam/sdk/common/proto_convert.hpp ../../viam/sdk/common/proto_value.hpp - ../../viam/sdk/common/service_helper.hpp ../../viam/sdk/common/utils.hpp ../../viam/sdk/common/version_metadata.hpp ../../viam/sdk/common/world_state.hpp diff --git a/src/viam/sdk/common/service_helper.cpp b/src/viam/sdk/common/private/service_helper.cpp similarity index 96% rename from src/viam/sdk/common/service_helper.cpp rename to src/viam/sdk/common/private/service_helper.cpp index f8f6a29f0..eb0607050 100644 --- a/src/viam/sdk/common/service_helper.cpp +++ b/src/viam/sdk/common/private/service_helper.cpp @@ -1,4 +1,4 @@ -#include +#include #include diff --git a/src/viam/sdk/common/service_helper.hpp b/src/viam/sdk/common/private/service_helper.hpp similarity index 100% rename from src/viam/sdk/common/service_helper.hpp rename to src/viam/sdk/common/private/service_helper.hpp diff --git a/src/viam/sdk/components/private/arm_server.cpp b/src/viam/sdk/components/private/arm_server.cpp index 505c2122c..7e1be85c9 100644 --- a/src/viam/sdk/components/private/arm_server.cpp +++ b/src/viam/sdk/components/private/arm_server.cpp @@ -1,6 +1,6 @@ #include -#include +#include namespace viam { namespace sdk { diff --git a/src/viam/sdk/components/private/base_server.cpp b/src/viam/sdk/components/private/base_server.cpp index 722a1f9ae..2a2cdd130 100644 --- a/src/viam/sdk/components/private/base_server.cpp +++ b/src/viam/sdk/components/private/base_server.cpp @@ -3,7 +3,7 @@ #include #include -#include +#include #include #include #include diff --git a/src/viam/sdk/components/private/board_server.cpp b/src/viam/sdk/components/private/board_server.cpp index 9b7338d65..c352b7589 100644 --- a/src/viam/sdk/components/private/board_server.cpp +++ b/src/viam/sdk/components/private/board_server.cpp @@ -1,7 +1,7 @@ #include #include -#include +#include #include #include #include diff --git a/src/viam/sdk/components/private/camera_server.cpp b/src/viam/sdk/components/private/camera_server.cpp index ab4b65179..c5d32b691 100644 --- a/src/viam/sdk/components/private/camera_server.cpp +++ b/src/viam/sdk/components/private/camera_server.cpp @@ -3,7 +3,7 @@ #include #include -#include +#include #include #include #include diff --git a/src/viam/sdk/components/private/encoder_server.cpp b/src/viam/sdk/components/private/encoder_server.cpp index a85cbf157..14273cd2f 100644 --- a/src/viam/sdk/components/private/encoder_server.cpp +++ b/src/viam/sdk/components/private/encoder_server.cpp @@ -1,7 +1,7 @@ #include #include -#include +#include #include #include #include diff --git a/src/viam/sdk/components/private/gantry_server.cpp b/src/viam/sdk/components/private/gantry_server.cpp index 124f6bcef..e0e59dde6 100644 --- a/src/viam/sdk/components/private/gantry_server.cpp +++ b/src/viam/sdk/components/private/gantry_server.cpp @@ -1,6 +1,6 @@ #include -#include +#include namespace viam { namespace sdk { diff --git a/src/viam/sdk/components/private/generic_server.cpp b/src/viam/sdk/components/private/generic_server.cpp index 75860c819..b226082ed 100644 --- a/src/viam/sdk/components/private/generic_server.cpp +++ b/src/viam/sdk/components/private/generic_server.cpp @@ -1,6 +1,6 @@ #include -#include +#include #include #include diff --git a/src/viam/sdk/components/private/gripper_server.cpp b/src/viam/sdk/components/private/gripper_server.cpp index 1675829af..3963c7fb4 100644 --- a/src/viam/sdk/components/private/gripper_server.cpp +++ b/src/viam/sdk/components/private/gripper_server.cpp @@ -1,6 +1,6 @@ #include -#include +#include namespace viam { namespace sdk { diff --git a/src/viam/sdk/components/private/motor_server.cpp b/src/viam/sdk/components/private/motor_server.cpp index 66f366fe6..7b7fd3a55 100644 --- a/src/viam/sdk/components/private/motor_server.cpp +++ b/src/viam/sdk/components/private/motor_server.cpp @@ -1,7 +1,7 @@ #include +#include #include -#include #include #include #include diff --git a/src/viam/sdk/components/private/movement_sensor_server.cpp b/src/viam/sdk/components/private/movement_sensor_server.cpp index 7f48a61e5..a0317a4f6 100644 --- a/src/viam/sdk/components/private/movement_sensor_server.cpp +++ b/src/viam/sdk/components/private/movement_sensor_server.cpp @@ -1,7 +1,7 @@ #include #include -#include +#include #include #include #include diff --git a/src/viam/sdk/components/private/pose_tracker_server.cpp b/src/viam/sdk/components/private/pose_tracker_server.cpp index 028acb775..4a7e5afdf 100644 --- a/src/viam/sdk/components/private/pose_tracker_server.cpp +++ b/src/viam/sdk/components/private/pose_tracker_server.cpp @@ -2,7 +2,7 @@ #include -#include +#include #include #include #include diff --git a/src/viam/sdk/components/private/power_sensor_server.cpp b/src/viam/sdk/components/private/power_sensor_server.cpp index 4d81f013e..e9f5ccc00 100644 --- a/src/viam/sdk/components/private/power_sensor_server.cpp +++ b/src/viam/sdk/components/private/power_sensor_server.cpp @@ -1,6 +1,6 @@ #include -#include +#include #include #include #include diff --git a/src/viam/sdk/components/private/sensor_server.cpp b/src/viam/sdk/components/private/sensor_server.cpp index 47301b040..2a8cd3755 100644 --- a/src/viam/sdk/components/private/sensor_server.cpp +++ b/src/viam/sdk/components/private/sensor_server.cpp @@ -1,6 +1,6 @@ #include -#include +#include #include #include #include diff --git a/src/viam/sdk/components/private/servo_server.cpp b/src/viam/sdk/components/private/servo_server.cpp index 831b57781..83f31ad60 100644 --- a/src/viam/sdk/components/private/servo_server.cpp +++ b/src/viam/sdk/components/private/servo_server.cpp @@ -1,6 +1,6 @@ #include -#include +#include #include #include #include diff --git a/src/viam/sdk/services/private/generic_server.cpp b/src/viam/sdk/services/private/generic_server.cpp index efecfcbfd..846560964 100644 --- a/src/viam/sdk/services/private/generic_server.cpp +++ b/src/viam/sdk/services/private/generic_server.cpp @@ -1,6 +1,6 @@ #include -#include +#include #include #include diff --git a/src/viam/sdk/services/private/mlmodel_server.cpp b/src/viam/sdk/services/private/mlmodel_server.cpp index 2e3d3c2c2..a19b1864a 100644 --- a/src/viam/sdk/services/private/mlmodel_server.cpp +++ b/src/viam/sdk/services/private/mlmodel_server.cpp @@ -14,7 +14,7 @@ #include -#include +#include #include #include #include diff --git a/src/viam/sdk/services/private/motion_server.cpp b/src/viam/sdk/services/private/motion_server.cpp index eee9360cf..ae09190ad 100644 --- a/src/viam/sdk/services/private/motion_server.cpp +++ b/src/viam/sdk/services/private/motion_server.cpp @@ -5,8 +5,8 @@ #include #include #include +#include #include -#include #include #include #include diff --git a/src/viam/sdk/services/private/navigation_server.cpp b/src/viam/sdk/services/private/navigation_server.cpp index d3764a3a0..75664b8c4 100644 --- a/src/viam/sdk/services/private/navigation_server.cpp +++ b/src/viam/sdk/services/private/navigation_server.cpp @@ -4,8 +4,8 @@ #include #include +#include #include -#include #include #include #include From 1b67b11a46b93e5740ab11c0d5178f8abfc999cf Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Tue, 7 Jan 2025 12:47:35 -0500 Subject: [PATCH 3/5] use shared_ptr and make dtors override = default --- src/viam/sdk/common/exception.cpp | 6 +----- src/viam/sdk/common/exception.hpp | 7 ++++--- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/src/viam/sdk/common/exception.cpp b/src/viam/sdk/common/exception.cpp index 3a76efde4..eaf288041 100644 --- a/src/viam/sdk/common/exception.cpp +++ b/src/viam/sdk/common/exception.cpp @@ -10,8 +10,6 @@ Exception::Exception(ErrorCondition condition, const std::string& what) Exception::Exception(const std::string& what) : Exception(ErrorCondition::k_general, what) {}; -Exception::~Exception() = default; - const std::error_condition& Exception::condition() const noexcept { return condition_; }; @@ -49,9 +47,7 @@ std::error_condition make_error_condition(ErrorCondition e) { GRPCException::GRPCException(const grpc::Status* status) : Exception(ErrorCondition::k_grpc, status->error_message()), - status_(std::make_unique(*status)) {} - -GRPCException::~GRPCException() = default; + status_(std::make_shared(*status)) {} const grpc::Status* GRPCException::status() const noexcept { return status_.get(); diff --git a/src/viam/sdk/common/exception.hpp b/src/viam/sdk/common/exception.hpp index adfdc6ecd..c43c8fda7 100644 --- a/src/viam/sdk/common/exception.hpp +++ b/src/viam/sdk/common/exception.hpp @@ -42,7 +42,8 @@ class Exception : public std::runtime_error { public: explicit Exception(ErrorCondition condition, const std::string& what); explicit Exception(const std::string& what); - virtual ~Exception(); + + ~Exception() override = default; const std::error_condition& condition() const noexcept; @@ -56,12 +57,12 @@ class Exception : public std::runtime_error { class GRPCException : public Exception { public: explicit GRPCException(const grpc::Status* status); - ~GRPCException(); + ~GRPCException() override = default; const grpc::Status* status() const noexcept; private: - std::unique_ptr status_; + std::shared_ptr status_; }; } // namespace sdk From 5723aa5061ccfadd5e29ea23925a88b47a2c8666 Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Tue, 7 Jan 2025 12:51:17 -0500 Subject: [PATCH 4/5] remove semicolons for linter --- src/viam/sdk/common/exception.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/viam/sdk/common/exception.cpp b/src/viam/sdk/common/exception.cpp index eaf288041..0ff9899e7 100644 --- a/src/viam/sdk/common/exception.cpp +++ b/src/viam/sdk/common/exception.cpp @@ -6,9 +6,9 @@ namespace viam { namespace sdk { Exception::Exception(ErrorCondition condition, const std::string& what) - : std::runtime_error("viam::sdk::Exception: " + what), condition_(condition) {}; + : std::runtime_error("viam::sdk::Exception: " + what), condition_(condition) {} -Exception::Exception(const std::string& what) : Exception(ErrorCondition::k_general, what) {}; +Exception::Exception(const std::string& what) : Exception(ErrorCondition::k_general, what) {} const std::error_condition& Exception::condition() const noexcept { return condition_; From 8671c5baad90a70b52b50b73c05fdde45f794ca5 Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Tue, 7 Jan 2025 13:27:10 -0500 Subject: [PATCH 5/5] fix equality typo --- src/viam/sdk/common/client_helper.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/viam/sdk/common/client_helper.cpp b/src/viam/sdk/common/client_helper.cpp index f59d56d9f..b05bb37fe 100644 --- a/src/viam/sdk/common/client_helper.cpp +++ b/src/viam/sdk/common/client_helper.cpp @@ -21,7 +21,7 @@ namespace client_helper_details { } bool isStatusCancelled(int status) noexcept { - return status = ::grpc::StatusCode::CANCELLED; + return status == ::grpc::StatusCode::CANCELLED; } } // namespace client_helper_details