From b16d0ea12ea51ece8c2d74402df46384bb8dd88c Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Thu, 29 Jul 2021 13:16:33 -0300 Subject: [PATCH 1/9] Revert "Revert "Updating client API to be able to remove pending requests (#1728)" (#1733)" This reverts commit d5f3d35fbe3127e30d4ee9d40f57c42584eeb830. --- rclcpp/include/rclcpp/client.hpp | 304 ++++++++++++++++++++++++----- rclcpp/test/rclcpp/test_client.cpp | 16 +- 2 files changed, 271 insertions(+), 49 deletions(-) diff --git a/rclcpp/include/rclcpp/client.hpp b/rclcpp/include/rclcpp/client.hpp index bd2d326012..ccbed49493 100644 --- a/rclcpp/include/rclcpp/client.hpp +++ b/rclcpp/include/rclcpp/client.hpp @@ -17,12 +17,14 @@ #include #include -#include +#include #include +#include // NOLINT, cpplint doesn't think this is a cpp std header #include #include #include #include +#include // NOLINT #include "rcl/client.h" #include "rcl/error_handling.h" @@ -178,6 +180,9 @@ template class Client : public ClientBase { public: + using Request = typename ServiceT::Request; + using Response = typename ServiceT::Response; + using SharedRequest = typename ServiceT::Request::SharedPtr; using SharedResponse = typename ServiceT::Response::SharedPtr; @@ -187,6 +192,7 @@ class Client : public ClientBase using SharedPromise = std::shared_ptr; using SharedPromiseWithRequest = std::shared_ptr; + using Future = std::future; using SharedFuture = std::shared_future; using SharedFutureWithRequest = std::shared_future>; @@ -195,6 +201,88 @@ class Client : public ClientBase RCLCPP_SMART_PTR_DEFINITIONS(Client) + /// A convenient Client::Future and request id pair. + class FutureAndRequestId + { +public: + FutureAndRequestId(Future impl, int64_t req_id) + : impl_(std::move(impl)), req_id_(req_id) + {} + + /// Allow implicit conversions to `std::future` by reference. + // TODO(ivanpauno): Maybe, deprecate this in favor of get_future() (?) + operator Future &() {return impl_;} + + /// Deprecated, use take_future() instead. + /** + * Allow implicit conversions to `std::future` by value. + * \deprecated + */ + [[deprecated("FutureAndRequestId: use take_future() instead of an implicit conversion")]] + operator Future() {return impl_;} + + /// Returns the internal `std::future`, moving it out. + /** + * After calling this method, the internal future gets invalidated. + */ + Future + take_future() {return impl_;} + + /// Getter for the internal future. + Future & + get_future() {return impl_;} + + /// Getter for the internal future. + const Future & + get_future() const {return impl_;} + + /// Returns the request id associated with this future. + int64_t get_request_id() const {return req_id_;} + + // delegate future like methods in the std::future impl_ + + /// See std::future::share(). + SharedFuture share() noexcept {return impl_.share();} + /// See std::future::get(). + SharedResponse get() {return impl_.get();} + /// See std::future::valid(). + bool valid() const noexcept {return impl_.valid();} + /// See std::future::wait(). + void wait() const {return impl_.wait();} + /// See std::future::wait_for(). + template + std::future_status wait_for( + const std::chrono::duration & timeout_duration) const + { + return impl_.wait_for(timeout_duration); + } + /// See std::future::wait_until(). + template + std::future_status wait_until( + const std::chrono::time_point & timeout_time) const + { + return impl_.wait_until(timeout_time); + } + + // Rule of five, we could use the rule of zero here, but better be explicit as some of the + // methods are deleted. + + /// Move constructor. + FutureAndRequestId(FutureAndRequestId && other) noexcept = default; + /// Deleted copy constructor, each instance is a unique owner of the future. + FutureAndRequestId(const FutureAndRequestId & other) = delete; + /// Move assignment. + FutureAndRequestId & operator=(FutureAndRequestId && other) noexcept = default; + /// Deleted copy assignment, each instance is a unique owner of the future. + FutureAndRequestId & operator=(const FutureAndRequestId & other) = delete; + /// Destructor. + ~FutureAndRequestId() = default; + +private: + Future impl_; + int64_t req_id_; + }; + /// Default constructor. /** * The constructor for a Client is almost never called directly. @@ -292,34 +380,83 @@ class Client : public ClientBase std::shared_ptr request_header, std::shared_ptr response) override { - std::unique_lock lock(pending_requests_mutex_); - auto typed_response = std::static_pointer_cast(response); - int64_t sequence_number = request_header->sequence_number; - // TODO(esteve) this should throw instead since it is not expected to happen in the first place - if (this->pending_requests_.count(sequence_number) == 0) { - RCUTILS_LOG_ERROR_NAMED( - "rclcpp", - "Received invalid sequence number. Ignoring..."); + auto opt = this->get_and_erase_pending_request(request_header->sequence_number); + if (!opt) { return; } - auto tuple = this->pending_requests_[sequence_number]; - auto call_promise = std::get<0>(tuple); - auto callback = std::get<1>(tuple); - auto future = std::get<2>(tuple); - this->pending_requests_.erase(sequence_number); - // Unlock here to allow the service to be called recursively from one of its callbacks. - lock.unlock(); - - call_promise->set_value(typed_response); - callback(future); + auto & value = *opt; + auto typed_response = std::static_pointer_cast( + std::move(response)); + if (std::holds_alternative(value)) { + auto & promise = std::get(value); + promise.set_value(std::move(typed_response)); + } else if (std::holds_alternative(value)) { + Promise promise; + promise.set_value(std::move(typed_response)); + const auto & callback = std::get(value); + callback(promise.get_future().share()); + } else if (std::holds_alternative>(value)) { + PromiseWithRequest promise; + const auto & pair = std::get>(value); + promise.set_value(std::make_pair(std::move(pair.second), std::move(typed_response))); + pair.first(promise.get_future().share()); + } } - SharedFuture + /// Send a request to the service server. + /** + * This method returns a `FutureAndRequestId` instance + * that can be passed to Executor::spin_until_future_complete() to + * wait until it has been completed. + * + * If the future never completes, + * e.g. the call to Executor::spin_until_future_complete() times out, + * Client::remove_pending_request() must be called to clean the client internal state. + * Not doing so will make the `Client` instance to use more memory each time a response is not + * received from the service server. + * + * ```cpp + * auto future = client->async_send_request(my_request); + * if ( + * rclcpp::FutureReturnCode::TIMEOUT == + * executor->spin_until_future_complete(future, timeout)) + * { + * client->remove_pending_request(future); + * // handle timeout + * } else { + * handle_response(future.get()); + * } + * ``` + * + * \param[in] request request to be send. + * \return a FutureAndRequestId instance. + */ + FutureAndRequestId async_send_request(SharedRequest request) { - return async_send_request(request, [](SharedFuture) {}); + Promise promise; + auto future = promise.get_future(); + auto req_id = async_send_request_impl( + *request, + std::move(promise)); + return FutureAndRequestId(std::move(future), req_id); } + /// Send a request to the service server and schedule a callback in the executor. + /** + * Similar to the previous overload, but a callback will automatically be called when a response is received. + * + * If the callback is never called, because we never got a reply for the service server, remove_pending_request() + * has to be called with the returned request id or prune_pending_requests(). + * Not doing so will make the `Client` instance use more memory each time a response is not + * received from the service server. + * In this case, it's convenient to setup a timer to cleanup the pending requests. + * + * \param[in] request request to be send. + * \param[in] cb callback that will be called when we get a response for this request. + * \return the request id representing the request just sent. + */ + // TODO(ivanpauno): Link to example that shows how to cleanup requests. template< typename CallbackT, typename std::enable_if< @@ -329,23 +466,24 @@ class Client : public ClientBase >::value >::type * = nullptr > - SharedFuture + int64_t async_send_request(SharedRequest request, CallbackT && cb) { - std::lock_guard lock(pending_requests_mutex_); - int64_t sequence_number; - rcl_ret_t ret = rcl_send_request(get_client_handle().get(), request.get(), &sequence_number); - if (RCL_RET_OK != ret) { - rclcpp::exceptions::throw_from_rcl_error(ret, "failed to send request"); - } - - SharedPromise call_promise = std::make_shared(); - SharedFuture f(call_promise->get_future()); - pending_requests_[sequence_number] = - std::make_tuple(call_promise, std::forward(cb), f); - return f; + return async_send_request_impl( + *request, + CallbackType{std::forward(cb)}); } + /// Send a request to the service server and schedule a callback in the executor. + /** + * Similar to the previous method, but you can get both the request and response in the callback. + * + * \param[in] request request to be send. + * \param[in] cb callback that will be called when we get a response for this request. + * \return the request id representing the request just sent. + */ + // TODO(ivanpauno): Deprecate this. + // If someone wants the request they can capture it in the lambda. template< typename CallbackT, typename std::enable_if< @@ -355,28 +493,100 @@ class Client : public ClientBase >::value >::type * = nullptr > - SharedFutureWithRequest + int64_t async_send_request(SharedRequest request, CallbackT && cb) { - SharedPromiseWithRequest promise = std::make_shared(); - SharedFutureWithRequest future_with_request(promise->get_future()); + auto & req = *request; + return async_send_request_impl( + req, + std::make_pair(CallbackWithRequestType{std::forward(cb)}, std::move(request))); + } + + /// Cleanup a pending request. + /** + * This notifies the client that we have waited long enough for a response from the server + * to come, we have given up and we are not waiting for a response anymore. + * + * Not calling this will make the client start using more memory for each request + * that never got a reply from the server. + * + * \param[in] request_id request id returned by async_send_request(). + * \return true when a pending request was removed, false if not (e.g. a response was received). + */ + bool + remove_pending_request(int64_t request_id) + { + std::lock_guard guard(pending_requests_mutex_); + return pending_requests_.erase(request_id) != 0u; + } + + /// Cleanup a pending request. + /** + * Convenient overload, same as: + * + * `Client::remove_pending_request(this, future.get_request_id())`. + */ + bool + remove_pending_request(const FutureAndRequestId & future) + { + return this->remove_pending_request(future.get_request_id()); + } + + /// Clean all pending requests. + /** + * \return number of pending requests that were removed. + */ + size_t + prune_requests() + { + std::lock_guard guard(pending_requests_mutex_); + auto ret = pending_requests_.size(); + pending_requests_.clear(); + return ret; + } - auto wrapping_cb = [future_with_request, promise, request, - cb = std::forward(cb)](SharedFuture future) { - auto response = future.get(); - promise->set_value(std::make_pair(request, response)); - cb(future_with_request); - }; +protected: + using PendingRequestsMapValue = std::variant< + std::promise, + CallbackType, + std::pair>; - async_send_request(request, wrapping_cb); + RCLCPP_PUBLIC + int64_t + async_send_request_impl(const Request & request, PendingRequestsMapValue value) + { + int64_t sequence_number; + rcl_ret_t ret = rcl_send_request(get_client_handle().get(), &request, &sequence_number); + if (RCL_RET_OK != ret) { + rclcpp::exceptions::throw_from_rcl_error(ret, "failed to send request"); + } + { + std::lock_guard lock(pending_requests_mutex_); + pending_requests_.try_emplace(sequence_number, std::move(value)); + } + return sequence_number; + } - return future_with_request; + RCLCPP_PUBLIC + std::optional + get_and_erase_pending_request(int64_t request_number) + { + std::unique_lock lock(pending_requests_mutex_); + auto it = this->pending_requests_.find(request_number); + if (it == this->pending_requests_.end()) { + RCUTILS_LOG_DEBUG_NAMED( + "rclcpp", + "Received invalid sequence number. Ignoring..."); + return std::nullopt; + } + auto value = std::move(it->second); + this->pending_requests_.erase(request_number); + return value; } -private: RCLCPP_DISABLE_COPY(Client) - std::map> pending_requests_; + std::unordered_map pending_requests_; std::mutex pending_requests_mutex_; }; diff --git a/rclcpp/test/rclcpp/test_client.cpp b/rclcpp/test/rclcpp/test_client.cpp index ea64340f03..5e2adb5e7d 100644 --- a/rclcpp/test/rclcpp/test_client.cpp +++ b/rclcpp/test/rclcpp/test_client.cpp @@ -216,7 +216,7 @@ class TestClientWithServer : public ::testing::Test received_response = true; }; - auto future = client->async_send_request(request, std::move(callback)); + auto req_id = client->async_send_request(request, std::move(callback)); auto start = std::chrono::steady_clock::now(); while (!received_response && @@ -228,6 +228,9 @@ class TestClientWithServer : public ::testing::Test if (!received_response) { return ::testing::AssertionFailure() << "Waiting for response timed out"; } + if (client->remove_pending_request(req_id)) { + return ::testing::AssertionFailure() << "Should not be able to remove a finished request"; + } return request_result; } @@ -256,7 +259,7 @@ TEST_F(TestClientWithServer, async_send_request_callback_with_request) { EXPECT_NE(nullptr, request_response_pair.second); received_response = true; }; - auto future = client->async_send_request(request, std::move(callback)); + auto req_id = client->async_send_request(request, std::move(callback)); auto start = std::chrono::steady_clock::now(); while (!received_response && @@ -265,6 +268,15 @@ TEST_F(TestClientWithServer, async_send_request_callback_with_request) { rclcpp::spin_some(node); } EXPECT_TRUE(received_response); + EXPECT_FALSE(client->remove_pending_request(req_id)); +} + +TEST_F(TestClientWithServer, test_client_remove_pending_request) { + auto client = node->create_client("no_service_server_available_here"); + auto request = std::make_shared(); + auto future = client->async_send_request(request); + + EXPECT_TRUE(client->remove_pending_request(future)); } TEST_F(TestClientWithServer, async_send_request_rcl_send_request_error) { From 1b0a413f2afed54cda7d5379d9c4a35c720384f3 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Thu, 29 Jul 2021 17:29:34 -0300 Subject: [PATCH 2/9] Please linters Signed-off-by: Ivan Santiago Paunovic --- rclcpp/include/rclcpp/client.hpp | 250 ++++++++++++++++++++----------- 1 file changed, 162 insertions(+), 88 deletions(-) diff --git a/rclcpp/include/rclcpp/client.hpp b/rclcpp/include/rclcpp/client.hpp index ccbed49493..9c0f40678b 100644 --- a/rclcpp/include/rclcpp/client.hpp +++ b/rclcpp/include/rclcpp/client.hpp @@ -47,6 +47,69 @@ namespace rclcpp { +namespace detail +{ +template typename FutureT, typename T> +struct FutureAndRequestId +{ + FutureT future; + int64_t request_id; + + FutureAndRequestId(FutureT impl, int64_t req_id) + : future(std::move(impl)), request_id(req_id) + {} + + /// Allow implicit conversions to `std::future` by reference. + // TODO(ivanpauno): Maybe, deprecate this in favor of get_future() (?) + operator FutureT&() {return this->future;} + + /// Deprecated, use the `future` member variable instead. + /** + * Allow implicit conversions to `std::future` by value. + * \deprecated + */ + [[deprecated("FutureAndRequestId: use .future instead of an implicit conversion")]] + operator FutureT() {return this->future;} + + // delegate future like methods in the std::future impl_ + + /// See std::future::get(). + T get() {return this->future.get();} + /// See std::future::valid(). + bool valid() const noexcept {return this->future.valid();} + /// See std::future::wait(). + void wait() const {return this->future.wait();} + /// See std::future::wait_for(). + template + std::future_status wait_for( + const std::chrono::duration & timeout_duration) const + { + return this->future.wait_for(timeout_duration); + } + /// See std::future::wait_until(). + template + std::future_status wait_until( + const std::chrono::time_point & timeout_time) const + { + return this->future.wait_until(timeout_time); + } + + // Rule of five, we could use the rule of zero here, but better be explicit as some of the + // methods are deleted. + + /// Move constructor. + FutureAndRequestId(FutureAndRequestId && other) noexcept = default; + /// Deleted copy constructor, each instance is a unique owner of the future. + FutureAndRequestId(const FutureAndRequestId & other) = delete; + /// Move assignment. + FutureAndRequestId & operator=(FutureAndRequestId && other) noexcept = default; + /// Deleted copy assignment, each instance is a unique owner of the future. + FutureAndRequestId & operator=(const FutureAndRequestId & other) = delete; + /// Destructor. + ~FutureAndRequestId() = default; +}; +} // namespace detail + namespace node_interfaces { class NodeBaseInterface; @@ -202,85 +265,62 @@ class Client : public ClientBase RCLCPP_SMART_PTR_DEFINITIONS(Client) /// A convenient Client::Future and request id pair. - class FutureAndRequestId + /** + * Public members: + * - future: a std::future. + * - request_id: the request id associated with the future. + * + * All the other methods are equivalent to the ones std::future provides. + */ + struct FutureAndRequestId + : detail::FutureAndRequestId { -public: - FutureAndRequestId(Future impl, int64_t req_id) - : impl_(std::move(impl)), req_id_(req_id) - {} + using detail::FutureAndRequestId::FutureAndRequestId; - /// Allow implicit conversions to `std::future` by reference. - // TODO(ivanpauno): Maybe, deprecate this in favor of get_future() (?) - operator Future &() {return impl_;} - - /// Deprecated, use take_future() instead. + /// Deprecated, use `.future.share()` instead. /** - * Allow implicit conversions to `std::future` by value. + * Allow implicit conversions to `std::shared_future` by value. * \deprecated */ - [[deprecated("FutureAndRequestId: use take_future() instead of an implicit conversion")]] - operator Future() {return impl_;} - - /// Returns the internal `std::future`, moving it out. - /** - * After calling this method, the internal future gets invalidated. - */ - Future - take_future() {return impl_;} - - /// Getter for the internal future. - Future & - get_future() {return impl_;} - - /// Getter for the internal future. - const Future & - get_future() const {return impl_;} - - /// Returns the request id associated with this future. - int64_t get_request_id() const {return req_id_;} + [[deprecated( + "FutureAndRequestId: use .future.share() instead of an implicit conversion")]] + operator SharedFuture() {return this->future.share();} // delegate future like methods in the std::future impl_ /// See std::future::share(). - SharedFuture share() noexcept {return impl_.share();} - /// See std::future::get(). - SharedResponse get() {return impl_.get();} - /// See std::future::valid(). - bool valid() const noexcept {return impl_.valid();} - /// See std::future::wait(). - void wait() const {return impl_.wait();} - /// See std::future::wait_for(). - template - std::future_status wait_for( - const std::chrono::duration & timeout_duration) const - { - return impl_.wait_for(timeout_duration); - } - /// See std::future::wait_until(). - template - std::future_status wait_until( - const std::chrono::time_point & timeout_time) const - { - return impl_.wait_until(timeout_time); - } + SharedFuture share() noexcept {return this->future.share();} + }; + + /// A convenient Client::SharedFuture and request id pair. + /** + * Public members: + * - future: a std::shared_future. + * - request_id: the request id associated with the future. + * + * All the other methods are equivalent to the ones std::shared_future provides. + */ + struct SharedFutureAndRequestId + : detail::FutureAndRequestId + { + using detail::FutureAndRequestId::FutureAndRequestId; + }; - // Rule of five, we could use the rule of zero here, but better be explicit as some of the - // methods are deleted. - - /// Move constructor. - FutureAndRequestId(FutureAndRequestId && other) noexcept = default; - /// Deleted copy constructor, each instance is a unique owner of the future. - FutureAndRequestId(const FutureAndRequestId & other) = delete; - /// Move assignment. - FutureAndRequestId & operator=(FutureAndRequestId && other) noexcept = default; - /// Deleted copy assignment, each instance is a unique owner of the future. - FutureAndRequestId & operator=(const FutureAndRequestId & other) = delete; - /// Destructor. - ~FutureAndRequestId() = default; - -private: - Future impl_; - int64_t req_id_; + /// A convenient Client::SharedFutureWithRequest and request id pair. + /** + * Public members: + * - future: a std::shared_future. + * - request_id: the request id associated with the future. + * + * All the other methods are equivalent to the ones std::shared_future provides. + */ + struct SharedFutureWithRequestAndRequestId + : detail::FutureAndRequestId> + { + using detail::FutureAndRequestId< + std::shared_future, + std::pair + >::FutureAndRequestId; }; /// Default constructor. @@ -390,16 +430,21 @@ class Client : public ClientBase if (std::holds_alternative(value)) { auto & promise = std::get(value); promise.set_value(std::move(typed_response)); - } else if (std::holds_alternative(value)) { - Promise promise; + } else if (std::holds_alternative(value)) { + auto & inner = std::get(value); + const auto & callback = std::get(inner); + auto & promise = std::get(inner); + auto & future = std::get(inner); promise.set_value(std::move(typed_response)); - const auto & callback = std::get(value); - callback(promise.get_future().share()); - } else if (std::holds_alternative>(value)) { - PromiseWithRequest promise; - const auto & pair = std::get>(value); - promise.set_value(std::make_pair(std::move(pair.second), std::move(typed_response))); - pair.first(promise.get_future().share()); + callback(std::move(future)); + } else if (std::holds_alternative(value)) { + auto & inner = std::get(value); + const auto & callback = std::get(inner); + auto & promise = std::get(inner); + auto & future = std::get(inner); + auto & request = std::get(inner); + promise.set_value(std::make_pair(std::move(request), std::move(typed_response))); + callback(future); } } @@ -466,12 +511,18 @@ class Client : public ClientBase >::value >::type * = nullptr > - int64_t + SharedFutureAndRequestId async_send_request(SharedRequest request, CallbackT && cb) { - return async_send_request_impl( + Promise promise; + auto shared_future = promise.get_future().share(); + auto req_id = async_send_request_impl( *request, - CallbackType{std::forward(cb)}); + std::make_tuple( + CallbackType{std::forward(cb)}, + shared_future, + std::move(promise))); + return SharedFutureAndRequestId{std::move(shared_future), req_id}; } /// Send a request to the service server and schedule a callback in the executor. @@ -493,13 +544,20 @@ class Client : public ClientBase >::value >::type * = nullptr > - int64_t + SharedFutureWithRequestAndRequestId async_send_request(SharedRequest request, CallbackT && cb) { auto & req = *request; - return async_send_request_impl( + PromiseWithRequest promise; + auto shared_future = promise.get_future().share(); + auto req_id = async_send_request_impl( req, - std::make_pair(CallbackWithRequestType{std::forward(cb)}, std::move(request))); + std::make_tuple( + CallbackWithRequestType{std::forward(cb)}, + std::move(request), + shared_future, + std::move(promise))); + return SharedFutureWithRequestAndRequestId{std::move(shared_future), req_id}; } /// Cleanup a pending request. @@ -529,7 +587,19 @@ class Client : public ClientBase bool remove_pending_request(const FutureAndRequestId & future) { - return this->remove_pending_request(future.get_request_id()); + return this->remove_pending_request(future.request_id); + } + + bool + remove_pending_request(const SharedFutureAndRequestId & future) + { + return this->remove_pending_request(future.request_id); + } + + bool + remove_pending_request(const SharedFutureWithRequestAndRequestId & future) + { + return this->remove_pending_request(future.request_id); } /// Clean all pending requests. @@ -537,7 +607,7 @@ class Client : public ClientBase * \return number of pending requests that were removed. */ size_t - prune_requests() + prune_pending_requests() { std::lock_guard guard(pending_requests_mutex_); auto ret = pending_requests_.size(); @@ -546,10 +616,14 @@ class Client : public ClientBase } protected: + using CallbackTypeValueVariant = std::tuple; + using CallbackWithRequestTypeValueVariant = std::tuple< + CallbackWithRequestType, SharedRequest, SharedFutureWithRequest, PromiseWithRequest>; + using PendingRequestsMapValue = std::variant< std::promise, - CallbackType, - std::pair>; + CallbackTypeValueVariant, + CallbackWithRequestTypeValueVariant>; RCLCPP_PUBLIC int64_t From d096d7b8191ce356d7a931f1f8f2f2f78ccd1501 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Thu, 29 Jul 2021 19:20:38 -0300 Subject: [PATCH 3/9] simplify code Signed-off-by: Ivan Santiago Paunovic --- rclcpp/include/rclcpp/client.hpp | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/rclcpp/include/rclcpp/client.hpp b/rclcpp/include/rclcpp/client.hpp index 9c0f40678b..5bd3dc715c 100644 --- a/rclcpp/include/rclcpp/client.hpp +++ b/rclcpp/include/rclcpp/client.hpp @@ -49,19 +49,19 @@ namespace rclcpp namespace detail { -template typename FutureT, typename T> +template struct FutureAndRequestId { - FutureT future; + FutureT future; int64_t request_id; - FutureAndRequestId(FutureT impl, int64_t req_id) + FutureAndRequestId(FutureT impl, int64_t req_id) : future(std::move(impl)), request_id(req_id) {} /// Allow implicit conversions to `std::future` by reference. // TODO(ivanpauno): Maybe, deprecate this in favor of get_future() (?) - operator FutureT&() {return this->future;} + operator FutureT&() {return this->future;} /// Deprecated, use the `future` member variable instead. /** @@ -69,12 +69,12 @@ struct FutureAndRequestId * \deprecated */ [[deprecated("FutureAndRequestId: use .future instead of an implicit conversion")]] - operator FutureT() {return this->future;} + operator FutureT() {return this->future;} // delegate future like methods in the std::future impl_ /// See std::future::get(). - T get() {return this->future.get();} + auto get() {return this->future.get();} /// See std::future::valid(). bool valid() const noexcept {return this->future.valid();} /// See std::future::wait(). @@ -273,9 +273,9 @@ class Client : public ClientBase * All the other methods are equivalent to the ones std::future provides. */ struct FutureAndRequestId - : detail::FutureAndRequestId + : detail::FutureAndRequestId> { - using detail::FutureAndRequestId::FutureAndRequestId; + using detail::FutureAndRequestId>::FutureAndRequestId; /// Deprecated, use `.future.share()` instead. /** @@ -301,9 +301,9 @@ class Client : public ClientBase * All the other methods are equivalent to the ones std::shared_future provides. */ struct SharedFutureAndRequestId - : detail::FutureAndRequestId + : detail::FutureAndRequestId> { - using detail::FutureAndRequestId::FutureAndRequestId; + using detail::FutureAndRequestId>::FutureAndRequestId; }; /// A convenient Client::SharedFutureWithRequest and request id pair. @@ -315,11 +315,10 @@ class Client : public ClientBase * All the other methods are equivalent to the ones std::shared_future provides. */ struct SharedFutureWithRequestAndRequestId - : detail::FutureAndRequestId> + : detail::FutureAndRequestId>> { using detail::FutureAndRequestId< - std::shared_future, - std::pair + std::shared_future> >::FutureAndRequestId; }; @@ -444,7 +443,7 @@ class Client : public ClientBase auto & future = std::get(inner); auto & request = std::get(inner); promise.set_value(std::make_pair(std::move(request), std::move(typed_response))); - callback(future); + callback(std::move(future)); } } From 14ad9d3966045a7ca308cd5e3fbf1a7be3ad712e Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Thu, 12 Aug 2021 09:20:49 -0300 Subject: [PATCH 4/9] More verbose type and name Signed-off-by: Ivan Santiago Paunovic --- rclcpp/include/rclcpp/client.hpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/rclcpp/include/rclcpp/client.hpp b/rclcpp/include/rclcpp/client.hpp index 5bd3dc715c..732d8a2c40 100644 --- a/rclcpp/include/rclcpp/client.hpp +++ b/rclcpp/include/rclcpp/client.hpp @@ -419,11 +419,12 @@ class Client : public ClientBase std::shared_ptr request_header, std::shared_ptr response) override { - auto opt = this->get_and_erase_pending_request(request_header->sequence_number); - if (!opt) { + std::optional + optional_pending_request = this->get_and_erase_pending_request(request_header->sequence_number); + if (!optional_pending_request) { return; } - auto & value = *opt; + auto & value = *optional_pending_request; auto typed_response = std::static_pointer_cast( std::move(response)); if (std::holds_alternative(value)) { From 77f2be0c28690b63ec7e4ccc1736dc3282105fa0 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 13 Aug 2021 16:12:10 -0300 Subject: [PATCH 5/9] Address peer review comments Signed-off-by: Ivan Santiago Paunovic --- rclcpp/include/rclcpp/client.hpp | 73 +++++++++++++++++++++++++------- 1 file changed, 57 insertions(+), 16 deletions(-) diff --git a/rclcpp/include/rclcpp/client.hpp b/rclcpp/include/rclcpp/client.hpp index 732d8a2c40..79824fe042 100644 --- a/rclcpp/include/rclcpp/client.hpp +++ b/rclcpp/include/rclcpp/client.hpp @@ -60,8 +60,7 @@ struct FutureAndRequestId {} /// Allow implicit conversions to `std::future` by reference. - // TODO(ivanpauno): Maybe, deprecate this in favor of get_future() (?) - operator FutureT&() {return this->future;} + operator FutureT &() {return this->future;} /// Deprecated, use the `future` member variable instead. /** @@ -419,7 +418,7 @@ class Client : public ClientBase std::shared_ptr request_header, std::shared_ptr response) override { - std::optional + std::optional optional_pending_request = this->get_and_erase_pending_request(request_header->sequence_number); if (!optional_pending_request) { return; @@ -496,12 +495,12 @@ class Client : public ClientBase * Not doing so will make the `Client` instance use more memory each time a response is not * received from the service server. * In this case, it's convenient to setup a timer to cleanup the pending requests. + * See for example the `examples_rclcpp_async_client` package in https://github.com/ros2/examples. * * \param[in] request request to be send. * \param[in] cb callback that will be called when we get a response for this request. * \return the request id representing the request just sent. */ - // TODO(ivanpauno): Link to example that shows how to cleanup requests. template< typename CallbackT, typename std::enable_if< @@ -533,8 +532,6 @@ class Client : public ClientBase * \param[in] cb callback that will be called when we get a response for this request. * \return the request id representing the request just sent. */ - // TODO(ivanpauno): Deprecate this. - // If someone wants the request they can capture it in the lambda. template< typename CallbackT, typename std::enable_if< @@ -547,14 +544,13 @@ class Client : public ClientBase SharedFutureWithRequestAndRequestId async_send_request(SharedRequest request, CallbackT && cb) { - auto & req = *request; PromiseWithRequest promise; auto shared_future = promise.get_future().share(); auto req_id = async_send_request_impl( - req, + *request, std::make_tuple( CallbackWithRequestType{std::forward(cb)}, - std::move(request), + request, shared_future, std::move(promise))); return SharedFutureWithRequestAndRequestId{std::move(shared_future), req_id}; @@ -582,7 +578,7 @@ class Client : public ClientBase /** * Convenient overload, same as: * - * `Client::remove_pending_request(this, future.get_request_id())`. + * `Client::remove_pending_request(this, future.request_id)`. */ bool remove_pending_request(const FutureAndRequestId & future) @@ -590,12 +586,24 @@ class Client : public ClientBase return this->remove_pending_request(future.request_id); } + /// Cleanup a pending request. + /** + * Convenient overload, same as: + * + * `Client::remove_pending_request(this, future.request_id)`. + */ bool remove_pending_request(const SharedFutureAndRequestId & future) { return this->remove_pending_request(future.request_id); } + /// Cleanup a pending request. + /** + * Convenient overload, same as: + * + * `Client::remove_pending_request(this, future.request_id)`. + */ bool remove_pending_request(const SharedFutureWithRequestAndRequestId & future) { @@ -615,19 +623,45 @@ class Client : public ClientBase return ret; } + /// Clean all pending requests older than a time_point. + /** + * \param[in] time_point Requests that were sent before this point are going to be removed. + * \param[inout] pruned_requests Removed requests id will be pushed to the vector + * if a pointer is provided. + * \return number of pending requests that were removed. + */ + template> + size_t + prune_requests_older_than( + std::chrono::time_point time_point, + std::vector * pruned_requests = nullptr) + { + std::lock_guard guard(pending_requests_mutex_); + auto old_size = pending_requests_.size(); + for (auto it = pending_requests_.begin(), last = pending_requests_.end(); it != last; ) { + if (it->second.first < time_point) { + pruned_requests->push_back(it->first); + it = pending_requests_.erase(it); + } else { + ++it; + } + } + return old_size - pending_requests_.size(); + } + protected: using CallbackTypeValueVariant = std::tuple; using CallbackWithRequestTypeValueVariant = std::tuple< CallbackWithRequestType, SharedRequest, SharedFutureWithRequest, PromiseWithRequest>; - using PendingRequestsMapValue = std::variant< + using CallbackInfoVariant = std::variant< std::promise, CallbackTypeValueVariant, CallbackWithRequestTypeValueVariant>; RCLCPP_PUBLIC int64_t - async_send_request_impl(const Request & request, PendingRequestsMapValue value) + async_send_request_impl(const Request & request, CallbackInfoVariant value) { int64_t sequence_number; rcl_ret_t ret = rcl_send_request(get_client_handle().get(), &request, &sequence_number); @@ -636,13 +670,15 @@ class Client : public ClientBase } { std::lock_guard lock(pending_requests_mutex_); - pending_requests_.try_emplace(sequence_number, std::move(value)); + pending_requests_.try_emplace( + sequence_number, + std::make_pair(std::chrono::system_clock::now(), std::move(value))); } return sequence_number; } RCLCPP_PUBLIC - std::optional + std::optional get_and_erase_pending_request(int64_t request_number) { std::unique_lock lock(pending_requests_mutex_); @@ -653,14 +689,19 @@ class Client : public ClientBase "Received invalid sequence number. Ignoring..."); return std::nullopt; } - auto value = std::move(it->second); + auto value = std::move(it->second.second); this->pending_requests_.erase(request_number); return value; } RCLCPP_DISABLE_COPY(Client) - std::unordered_map pending_requests_; + std::unordered_map< + int64_t, + std::pair< + std::chrono::time_point, + CallbackInfoVariant>> + pending_requests_; std::mutex pending_requests_mutex_; }; From 0a1d8d5b621ac2d6bd567f612e56975d25589761 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Wed, 18 Aug 2021 16:10:05 -0300 Subject: [PATCH 6/9] Include what you use. Signed-off-by: Ivan Santiago Paunovic --- rclcpp/include/rclcpp/client.hpp | 1 + 1 file changed, 1 insertion(+) diff --git a/rclcpp/include/rclcpp/client.hpp b/rclcpp/include/rclcpp/client.hpp index 79824fe042..718adebcae 100644 --- a/rclcpp/include/rclcpp/client.hpp +++ b/rclcpp/include/rclcpp/client.hpp @@ -25,6 +25,7 @@ #include #include #include // NOLINT +#include #include "rcl/client.h" #include "rcl/error_handling.h" From 83707365d46604c74a747951ddbcfb73a9ceabad Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Wed, 18 Aug 2021 16:11:04 -0300 Subject: [PATCH 7/9] Fix tests in rclcpp_components Signed-off-by: Ivan Santiago Paunovic --- .../test/test_component_manager_api.cpp | 155 ++++++++++-------- 1 file changed, 84 insertions(+), 71 deletions(-) diff --git a/rclcpp_components/test/test_component_manager_api.cpp b/rclcpp_components/test/test_component_manager_api.cpp index 73d4bc3687..84b11fe7e9 100644 --- a/rclcpp_components/test/test_component_manager_api.cpp +++ b/rclcpp_components/test/test_component_manager_api.cpp @@ -57,13 +57,14 @@ TEST_F(TestComponentManager, components_api) request->package_name = "rclcpp_components"; request->plugin_name = "test_rclcpp_components::TestComponentFoo"; - auto result = composition_client->async_send_request(request); - auto ret = exec->spin_until_future_complete(result, 5s); // Wait for the result. + auto future = composition_client->async_send_request(request); + auto ret = exec->spin_until_future_complete(future, 5s); // Wait for the result. + auto result = future.get(); EXPECT_EQ(ret, rclcpp::FutureReturnCode::SUCCESS); - EXPECT_EQ(result.get()->success, true); - EXPECT_EQ(result.get()->error_message, ""); - EXPECT_EQ(result.get()->full_node_name, "/test_component_foo"); - EXPECT_EQ(result.get()->unique_id, 1u); + EXPECT_EQ(result->success, true); + EXPECT_EQ(result->error_message, ""); + EXPECT_EQ(result->full_node_name, "/test_component_foo"); + EXPECT_EQ(result->unique_id, 1u); } { @@ -71,13 +72,14 @@ TEST_F(TestComponentManager, components_api) request->package_name = "rclcpp_components"; request->plugin_name = "test_rclcpp_components::TestComponentBar"; - auto result = composition_client->async_send_request(request); - auto ret = exec->spin_until_future_complete(result, 5s); // Wait for the result. + auto future = composition_client->async_send_request(request); + auto ret = exec->spin_until_future_complete(future, 5s); // Wait for the result. + auto result = future.get(); EXPECT_EQ(ret, rclcpp::FutureReturnCode::SUCCESS); - EXPECT_EQ(result.get()->success, true); - EXPECT_EQ(result.get()->error_message, ""); - EXPECT_EQ(result.get()->full_node_name, "/test_component_bar"); - EXPECT_EQ(result.get()->unique_id, 2u); + EXPECT_EQ(result->success, true); + EXPECT_EQ(result->error_message, ""); + EXPECT_EQ(result->full_node_name, "/test_component_bar"); + EXPECT_EQ(result->unique_id, 2u); } // Test remapping the node name @@ -87,13 +89,14 @@ TEST_F(TestComponentManager, components_api) request->plugin_name = "test_rclcpp_components::TestComponentFoo"; request->node_name = "test_component_baz"; - auto result = composition_client->async_send_request(request); - auto ret = exec->spin_until_future_complete(result, 5s); // Wait for the result. + auto future = composition_client->async_send_request(request); + auto ret = exec->spin_until_future_complete(future, 5s); // Wait for the result. + auto result = future.get(); EXPECT_EQ(ret, rclcpp::FutureReturnCode::SUCCESS); - EXPECT_EQ(result.get()->success, true); - EXPECT_EQ(result.get()->error_message, ""); - EXPECT_EQ(result.get()->full_node_name, "/test_component_baz"); - EXPECT_EQ(result.get()->unique_id, 3u); + EXPECT_EQ(result->success, true); + EXPECT_EQ(result->error_message, ""); + EXPECT_EQ(result->full_node_name, "/test_component_baz"); + EXPECT_EQ(result->unique_id, 3u); } // Test remapping the node namespace @@ -104,13 +107,14 @@ TEST_F(TestComponentManager, components_api) request->node_namespace = "/ns"; request->node_name = "test_component_bing"; - auto result = composition_client->async_send_request(request); - auto ret = exec->spin_until_future_complete(result, 5s); // Wait for the result. + auto future = composition_client->async_send_request(request); + auto ret = exec->spin_until_future_complete(future, 5s); // Wait for the result. + auto result = future.get(); EXPECT_EQ(ret, rclcpp::FutureReturnCode::SUCCESS); - EXPECT_EQ(result.get()->success, true); - EXPECT_EQ(result.get()->error_message, ""); - EXPECT_EQ(result.get()->full_node_name, "/ns/test_component_bing"); - EXPECT_EQ(result.get()->unique_id, 4u); + EXPECT_EQ(result->success, true); + EXPECT_EQ(result->error_message, ""); + EXPECT_EQ(result->full_node_name, "/ns/test_component_bing"); + EXPECT_EQ(result->unique_id, 4u); } { @@ -119,13 +123,14 @@ TEST_F(TestComponentManager, components_api) request->package_name = "rclcpp_components"; request->plugin_name = "test_rclcpp_components::TestComponent"; - auto result = composition_client->async_send_request(request); - auto ret = exec->spin_until_future_complete(result, 5s); // Wait for the result. + auto future = composition_client->async_send_request(request); + auto ret = exec->spin_until_future_complete(future, 5s); // Wait for the result. EXPECT_EQ(ret, rclcpp::FutureReturnCode::SUCCESS); - EXPECT_EQ(result.get()->success, false); - EXPECT_EQ(result.get()->error_message, "Failed to find class with the requested plugin name."); - EXPECT_EQ(result.get()->full_node_name, ""); - EXPECT_EQ(result.get()->unique_id, 0u); + auto result = future.get(); + EXPECT_EQ(result->success, false); + EXPECT_EQ(result->error_message, "Failed to find class with the requested plugin name."); + EXPECT_EQ(result->full_node_name, ""); + EXPECT_EQ(result->unique_id, 0u); } { @@ -134,13 +139,14 @@ TEST_F(TestComponentManager, components_api) request->package_name = "rclcpp_components_foo"; request->plugin_name = "test_rclcpp_components::TestComponentFoo"; - auto result = composition_client->async_send_request(request); - auto ret = exec->spin_until_future_complete(result, 5s); // Wait for the result. + auto future = composition_client->async_send_request(request); + auto ret = exec->spin_until_future_complete(future, 5s); // Wait for the result. EXPECT_EQ(ret, rclcpp::FutureReturnCode::SUCCESS); - EXPECT_EQ(result.get()->success, false); - EXPECT_EQ(result.get()->error_message, "Could not find requested resource in ament index"); - EXPECT_EQ(result.get()->full_node_name, ""); - EXPECT_EQ(result.get()->unique_id, 0u); + auto result = future.get(); + EXPECT_EQ(result->success, false); + EXPECT_EQ(result->error_message, "Could not find requested resource in ament index"); + EXPECT_EQ(result->full_node_name, ""); + EXPECT_EQ(result->unique_id, 0u); } { @@ -151,13 +157,14 @@ TEST_F(TestComponentManager, components_api) request->node_name = "test_component_remap"; request->remap_rules.push_back("alice:=bob"); - auto result = composition_client->async_send_request(request); - auto ret = exec->spin_until_future_complete(result, 5s); // Wait for the result. + auto future = composition_client->async_send_request(request); + auto ret = exec->spin_until_future_complete(future, 5s); // Wait for the result. + auto result = future.get(); EXPECT_EQ(ret, rclcpp::FutureReturnCode::SUCCESS); - EXPECT_EQ(result.get()->success, true); - EXPECT_EQ(result.get()->error_message, ""); - EXPECT_EQ(result.get()->full_node_name, "/test_component_remap"); - EXPECT_EQ(result.get()->unique_id, 5u); + EXPECT_EQ(result->success, true); + EXPECT_EQ(result->error_message, ""); + EXPECT_EQ(result->full_node_name, "/test_component_remap"); + EXPECT_EQ(result->unique_id, 5u); } { @@ -170,14 +177,15 @@ TEST_F(TestComponentManager, components_api) rclcpp::ParameterValue(true)); request->extra_arguments.push_back(use_intraprocess_comms.to_parameter_msg()); - auto result = composition_client->async_send_request(request); - auto ret = exec->spin_until_future_complete(result, 5s); // Wait for the result. + auto future = composition_client->async_send_request(request); + auto ret = exec->spin_until_future_complete(future, 5s); // Wait for the result. + auto result = future.get(); EXPECT_EQ(ret, rclcpp::FutureReturnCode::SUCCESS); - EXPECT_EQ(result.get()->success, true); - EXPECT_EQ(result.get()->error_message, ""); - std::cout << result.get()->full_node_name << std::endl; - EXPECT_EQ(result.get()->full_node_name, "/test_component_intra_process"); - EXPECT_EQ(result.get()->unique_id, 6u); + EXPECT_EQ(result->success, true); + EXPECT_EQ(result->error_message, ""); + std::cout << result->full_node_name << std::endl; + EXPECT_EQ(result->full_node_name, "/test_component_intra_process"); + EXPECT_EQ(result->unique_id, 6u); } { @@ -191,15 +199,16 @@ TEST_F(TestComponentManager, components_api) rclcpp::ParameterValue("hello")); request->extra_arguments.push_back(use_intraprocess_comms.to_parameter_msg()); - auto result = composition_client->async_send_request(request); - auto ret = exec->spin_until_future_complete(result, 5s); // Wait for the result. + auto future = composition_client->async_send_request(request); + auto ret = exec->spin_until_future_complete(future, 5s); // Wait for the result. + auto result = future.get(); EXPECT_EQ(ret, rclcpp::FutureReturnCode::SUCCESS); - EXPECT_EQ(result.get()->success, false); + EXPECT_EQ(result->success, false); EXPECT_EQ( - result.get()->error_message, + result->error_message, "Extra component argument 'use_intra_process_comms' must be a boolean"); - EXPECT_EQ(result.get()->full_node_name, ""); - EXPECT_EQ(result.get()->unique_id, 0u); + EXPECT_EQ(result->full_node_name, ""); + EXPECT_EQ(result->unique_id, 0u); } auto node_names = node->get_node_names(); @@ -223,11 +232,12 @@ TEST_F(TestComponentManager, components_api) { auto request = std::make_shared(); - auto result = client->async_send_request(request); - auto ret = exec->spin_until_future_complete(result, 5s); // Wait for the result. + auto future = client->async_send_request(request); + auto ret = exec->spin_until_future_complete(future, 5s); // Wait for the result. EXPECT_EQ(ret, rclcpp::FutureReturnCode::SUCCESS); - auto result_node_names = result.get()->full_node_names; - auto result_unique_ids = result.get()->unique_ids; + auto result = future.get(); + auto result_node_names = result->full_node_names; + auto result_unique_ids = result->unique_ids; EXPECT_EQ(result_node_names.size(), 6u); EXPECT_EQ(result_node_names[0], "/test_component_foo"); @@ -258,22 +268,24 @@ TEST_F(TestComponentManager, components_api) auto request = std::make_shared(); request->unique_id = 1; - auto result = client->async_send_request(request); - auto ret = exec->spin_until_future_complete(result, 5s); // Wait for the result. + auto future = client->async_send_request(request); + auto ret = exec->spin_until_future_complete(future, 5s); // Wait for the result. + auto result = future.get(); EXPECT_EQ(ret, rclcpp::FutureReturnCode::SUCCESS); - EXPECT_EQ(result.get()->success, true); - EXPECT_EQ(result.get()->error_message, ""); + EXPECT_EQ(result->success, true); + EXPECT_EQ(result->error_message, ""); } { auto request = std::make_shared(); request->unique_id = 1; - auto result = client->async_send_request(request); - auto ret = exec->spin_until_future_complete(result, 5s); // Wait for the result. + auto future = client->async_send_request(request); + auto ret = exec->spin_until_future_complete(future, 5s); // Wait for the result. + auto result = future.get(); EXPECT_EQ(ret, rclcpp::FutureReturnCode::SUCCESS); - EXPECT_EQ(result.get()->success, false); - EXPECT_EQ(result.get()->error_message, "No node found with unique_id: 1"); + EXPECT_EQ(result->success, false); + EXPECT_EQ(result->error_message, "No node found with unique_id: 1"); } } @@ -287,11 +299,12 @@ TEST_F(TestComponentManager, components_api) { auto request = std::make_shared(); - auto result = client->async_send_request(request); - auto ret = exec->spin_until_future_complete(result, 5s); // Wait for the result. + auto future = client->async_send_request(request); + auto ret = exec->spin_until_future_complete(future, 5s); // Wait for the result. EXPECT_EQ(ret, rclcpp::FutureReturnCode::SUCCESS); - auto result_node_names = result.get()->full_node_names; - auto result_unique_ids = result.get()->unique_ids; + auto result = future.get(); + auto result_node_names = result->full_node_names; + auto result_unique_ids = result->unique_ids; EXPECT_EQ(result_node_names.size(), 5u); EXPECT_EQ(result_node_names[0], "/test_component_bar"); From 4ed3b3a4502cd7e26d1d03245df78d78baf9278a Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Wed, 18 Aug 2021 16:24:44 -0300 Subject: [PATCH 8/9] Fix rclcpp_lifecycle test cases Signed-off-by: Ivan Santiago Paunovic --- .../test/test_lifecycle_service_client.cpp | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/rclcpp_lifecycle/test/test_lifecycle_service_client.cpp b/rclcpp_lifecycle/test/test_lifecycle_service_client.cpp index d7188ca350..6dd2c0ec17 100644 --- a/rclcpp_lifecycle/test/test_lifecycle_service_client.cpp +++ b/rclcpp_lifecycle/test/test_lifecycle_service_client.cpp @@ -98,8 +98,9 @@ class LifecycleServiceClient : public rclcpp::Node return unknown_state; } - if (future_result.get()) { - return future_result.get()->current_state; + auto result = future_result.get(); + if (result) { + return result->current_state; } else { return unknown_state; } @@ -140,9 +141,9 @@ class LifecycleServiceClient : public rclcpp::Node if (future_status != std::future_status::ready) { return std::vector(); } - - if (future_result.get()) { - return future_result.get()->available_states; + auto result = future_result.get(); + if (result) { + return result->available_states; } return std::vector(); @@ -164,8 +165,9 @@ class LifecycleServiceClient : public rclcpp::Node return std::vector(); } - if (future_result.get()) { - return future_result.get()->available_transitions; + auto result = future_result.get(); + if (result) { + return result->available_transitions; } return std::vector(); @@ -187,8 +189,9 @@ class LifecycleServiceClient : public rclcpp::Node return std::vector(); } - if (future_result.get()) { - return future_result.get()->available_transitions; + auto result = future_result.get(); + if (result) { + return result->available_transitions; } return std::vector(); From e911233b8d2506b04235d297b5589c1a4d132d12 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Thu, 19 Aug 2021 13:19:07 -0300 Subject: [PATCH 9/9] Maybe fixes windows: don't use RCLCPP_PUBLIC in templates Signed-off-by: Ivan Santiago Paunovic --- rclcpp/include/rclcpp/client.hpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/rclcpp/include/rclcpp/client.hpp b/rclcpp/include/rclcpp/client.hpp index 718adebcae..1fc761f491 100644 --- a/rclcpp/include/rclcpp/client.hpp +++ b/rclcpp/include/rclcpp/client.hpp @@ -660,7 +660,6 @@ class Client : public ClientBase CallbackTypeValueVariant, CallbackWithRequestTypeValueVariant>; - RCLCPP_PUBLIC int64_t async_send_request_impl(const Request & request, CallbackInfoVariant value) { @@ -678,7 +677,6 @@ class Client : public ClientBase return sequence_number; } - RCLCPP_PUBLIC std::optional get_and_erase_pending_request(int64_t request_number) {