Calling a dead service causes a memory leak on the client side

[umateusz]

Bug report

If the service is stopped (due to an error or user intervention) and the client keeps trying to call it, it will leak memory. Unprocessed requests will be accumulated in pending_requests_ (

rclcpp/rclcpp/include/rclcpp/client.hpp

Line 379 in 7d8b269

std::map<int64_t, std::tuple<SharedPromise, CallbackType, SharedFuture>> pending_requests_;

). Is there a way to prevent this e.g. request timeout?

It should not be possible to leak memory on the client side. User cannot delete old requests, so they will hang until the end of the client's operation. Moreover, the problem will occur if the service does not keep up with the processing of client requests.

IMHO: Using spin_until_future_complete is not a solution as it cannot be called from a composable node and it cannot erase outdated entry from pending_request_ collection.

• Operating System: Ubuntu 20.04
• Installation type: binaries
• Version or commit hash: foxy
• DDS implementation: rmw_cyclonedds_cpp (should also be present for other implementations)
• Client library (if applicable): rclcpp

Steps to reproduce issue

I prepared the problem example by modifying the standard (minimal_client, minimal_service) examples and placed it on the branch: https://github.com/umateusz/examples/tree/memory-leak-in-service-client

Client:

#include <chrono>
#include <cinttypes>
#include <memory>
#include "example_interfaces/srv/add_two_ints.hpp"
#include "rclcpp/rclcpp.hpp"

using AddTwoInts = example_interfaces::srv::AddTwoInts;
using namespace std::chrono_literals;

class ClientWithTimer : public rclcpp::Node
{
public:
  ClientWithTimer() : Node("client_with_timer")
  {
    client_ = create_client<AddTwoInts>("add_two_ints");
    while (!client_->wait_for_service(std::chrono::seconds(1))) {
      if (!rclcpp::ok()) {
        RCLCPP_ERROR(get_logger(), "client interrupted while waiting for service to appear.");
        rclcpp::shutdown();
        return;
      }
      RCLCPP_INFO(get_logger(), "waiting for service to appear...");
    }
    timer_ = create_wall_timer(500ms, std::bind(&ClientWithTimer::timer_callback, this));
  }

private:
  void timer_callback()
  {
    RCLCPP_INFO(this->get_logger(), "Hello, world!");
    auto request = std::make_shared<AddTwoInts::Request>();
    request->a = 41;
    request->b = 1;
    auto result_future = client_->async_send_request(
      request, [this, request](rclcpp::Client<AddTwoInts>::SharedFuture response) {
        RCLCPP_INFO(this->get_logger(), "Hello from response handler!");
        auto result = response.get();
        RCLCPP_INFO(
          get_logger(), "result of %" PRId64 " + %" PRId64 " = %" PRId64, request->a, request->b,
          result->sum);
      });
  }
  rclcpp::TimerBase::SharedPtr timer_;
  rclcpp::Client<AddTwoInts>::SharedPtr client_;
};

int main(int argc, char * argv[])
{
  rclcpp::init(argc, argv);
  rclcpp::spin(std::make_shared<ClientWithTimer>());
  rclcpp::shutdown();
  return 0;
}

Service:

#include <inttypes.h>
#include <memory>
#include "example_interfaces/srv/add_two_ints.hpp"
#include "rclcpp/rclcpp.hpp"

using AddTwoInts = example_interfaces::srv::AddTwoInts;
rclcpp::Node::SharedPtr g_node = nullptr;

void handle_service(
  const std::shared_ptr<rmw_request_id_t> request_header,
  const std::shared_ptr<AddTwoInts::Request> request,
  const std::shared_ptr<AddTwoInts::Response> response)
{
  throw std::runtime_error("Some error in service");
  (void)request_header;
  RCLCPP_INFO(
    g_node->get_logger(),
    "request: %" PRId64 " + %" PRId64, request->a, request->b);
  response->sum = request->a + request->b;
}

int main(int argc, char ** argv)
{
  rclcpp::init(argc, argv);
  g_node = rclcpp::Node::make_shared("minimal_service");
  auto server = g_node->create_service<AddTwoInts>("add_two_ints", handle_service);
  rclcpp::spin(g_node);
  rclcpp::shutdown();
  g_node = nullptr;
  return 0;
}

[wjwwood]

This is definitely a bug, thanks for pointing it out @umateusz.

This will be difficult to resolve, because there's no way with the current signatures that are available to automatically remove a pending request. If we have a signature with a timeout then we can periodically prune the pending requests (maybe when handling a request we can prune) or we could provide a new method like prune_expired_pending_requests().

This would mean new signatures of send_request() (all versions, async, etc) that require timeouts be given, and probably deprecating the signatures that do not require timeouts.

---

In the meantime, I could imagine two things we could do.

First we could find a way to associate the returned SharedFuture with the sequence number used to index the pending requests, then we could have a new method like remove_pending_request(SharedFuture). Then at least the user could manually remove items that are pending if they wanted. This might be a good option for the long term too, so we can continue to have a signature of send_request that requires no timeout, but instead relies on the user to remove it from the pending requests themselves.

Second we could have a new method like remove_all_pending_requests(). This option is less useful, but could be a workaround for the memory leak that could also be backported without much concern.

Another weird option would be to track the time that each request was sent (a map from sequence number to timestamps) and then a method like remove_pending_requests_older_than(duration), which would iterate through them and remove the ones that are old enough. That might be slightly more useful that "remove all", but also may not be backportable due to ABI breaking.

---

Either way, we'd appreciate help with this if anyone is inclined.

@ivanpauno @fujitatomoya in case you guys have opinions.

[wjwwood]

At the same time as this, we should have a error handling case to go with the "normal" callback in the async_send_request() that takes a callback. Timeout can be a special case of "an error occurred".

[ralwing]

IMHO the best solution here is to implement self-holding request, and completely remove the pending request collection.
Having a fire and forget like implementation doesn't require holding the pending requests.
It's similar to how the boost::asio::dispatch works.

[ivanpauno]

@ivanpauno @fujitatomoya in case you guys have opinions.

I realized of this exact same issue when working on the domain bridge, the current "wait for an available service and then send request" approach is naturally racy.

IMO async_send_request() should receive a timeout.
My idea was to have something like:

  client->send_async_request(request, timeout, handle_request_cb, handle_timeout_cb);

First we could find a way to associate the returned SharedFuture with the sequence number used to index the pending requests, then we could have a new method like remove_pending_request(SharedFuture)

That sounds fine to me, if we have that people could use spin_until_future_complete(future_response, timeout) and delete the request if the timeout expired.
Though I would really prefer to also have a new signature, which looks like much more powerful.

---

It would also be great if instead of only deleting a request after a timeout expires, we could also detect that the service went down and get a more specific notification (server disconected).
That unfortunately requires much more work, and I think that the timeout alternative is "good enough" in practice.

[ivanpauno]

Updated my last comment, as I clicked the "comment" button by mistake

[wjwwood]

IMHO the best solution here is to implement self-holding request, and completely remove the pending request collection.
Having a fire and forget like implementation doesn't require holding the pending requests.
It's similar to how the boost::asio::dispatch works.

That could be an option too. I'd have to think about it more, but whoever ends up working on this should at least consider it.

[ivanpauno]

I'm currently working in trying to fix this.
It doesn't seem to be super simple though (it requires some refactoring of the current code), so as soon as I have something minimal working correctly I will open a draft PR for feedback.

[fujitatomoya]

@Barry-Xu-2018 @iuhilnehc-ynos

this is memory leak, could you guys take a look at this when you have time?

[ivanpauno]

@Barry-Xu-2018 @iuhilnehc-ynos

this is memory leak, could you guys take a look at this when you have time?

FYI, I've opened #1728, which extends the Client API to be able to cleanup pending requests.