Rclcpp: Possible memory leak in spin_some()

[BriceRenaudeau]

Bug report

When using rclcpp::spin_some(node);, the node resources increase forever.

Required Info:

• Operating System:
  
  
  
  • Ubuntu 20.04
  
  
• Installation type:
  
  
  
  • binaries
  
  
• Version or commit hash:
  
  
  
  • Rolling
  
  
• DDS implementation:
  
  
  
  • Fast-RTPS or CycloneDDS
  
  
• Client library (if applicable):
  
  
  
  • rclcpp
  
  

Steps to reproduce issue

Compile and execute this simple node.

#include <rclcpp/rclcpp.hpp>
int main(int argc, char * argv[])
{
  rclcpp::init(argc, argv);
  auto node = std::make_shared<rclcpp::Node>("foo");
  while(rclcpp::ok())
  {
    rclcpp::spin_some(node);
  }

  rclcpp::shutdown();
  return 0;
}

Using htop you will see the resources of the process increase.

Expected behavior

The node resources should be stable.

Actual behavior

The node resources increase until the OS kills the node because of memory storage.
Out of memory: Killed process

Additional information

It can be easily bypassed by creating an executor out of the loop and spinning it inside.

[clalancette]

• Version or commit hash:

  • release version

Can you be more specific on what version you are using? Is it Foxy, is it Rolling?

[BriceRenaudeau]

Sorry, I am using Rolling.

[clalancette]

OK, yeah, confirmed. It is definitely leaking in the latest from-source build as well.

[fujitatomoya]

confirmed this happens with https://github.com/ros2/ros2/commit/2c3dcb37040365ee5f09eede264cd05910134076. (cyclonedds and fastdds)

(gdb) info proc mappings
process 11010
Mapped address spaces:
...<snip>
      0x555555561000     0x55555ead0000  0x956f000        0x0 [heap]

(gdb) whatis 0x555555561000
type = long

[fujitatomoya]

@Barry-Xu-2018 @iuhilnehc-ynos i guess this is high priority, we would want to take a look.

[Barry-Xu-2018]

@Barry-Xu-2018 @iuhilnehc-ynos i guess this is high priority, we would want to take a look.

Okay. We check it now.

[Barry-Xu-2018]

The cause is related to below codes

https://github.com/ros2/rclcpp/blob/b8df9347a10bff383fdc8890c5b1bfdf16430d8f/rclcpp/src/rclcpp/executor.cpp#L59-L65

In the construct of Executor, one callback function is added to on_shutdown_callbacks_ of Context
But in the destructor of Executor, this callback isn't removed from on_shutdown_callbacks_.

Test codes is

  while(rclcpp::ok())
  {
    rclcpp::spin_some(node);
  }

Executor is created continuously, so callback is added continuously to on_shutdown_callbacks_ of Context.

About the solution, below is one way.

std::vector<OnShutdownCallback> on_shutdown_callbacks_;

Change to

std::map<Executor *, OnShutdownCallback> on_shutdown_callbacks_;

In the destructor of Executor, we remove callback based on the pointer of Executor.
But this way leads to many changes, such as

https://github.com/ros2/rclcpp/blob/b8df9347a10bff383fdc8890c5b1bfdf16430d8f/rclcpp/src/rclcpp/context.cpp#L344-L361

In get_on_shutdown_callbacks, we have to make a vector based on the map.

Does anybody have another good idea on the solution ?

[fujitatomoya]

ah, i was one step behind... :cry:

i was going to say that. actually i believe that is correct, i confirmed that using gdb.

(gdb) print on_shutdown_callbacks_
$7 = std::vector of length 1098618, capacity 2097152 = {
...

really huge list...

[fujitatomoya]

In the destructor of Executor, we remove callback based on the pointer of Executor.

agree, this needs to be done. and shutdown_guard_condition_ is shared with weak_ptr, so if Executor is already gone, this callbacks does not do anything via context shutdown.

Does anybody have another good idea on the solution ?

i don't have it either. (instead of suggested approach.)

[fujitatomoya]

@wjwwood @clalancette would you share your thoughts how we wanna go? after that, we can work on the fix. besides, i guess that this should be fixed in Galactic, but currently it requires to break ABI, i think.

[clalancette]

As a general comment, it is still fine to break ABI for bugfixes at this time. So there is no trouble about doing that.

I need to look at this in more detail to see if I have any other ideas for solving it.

[clalancette]

In the destructor of Executor, we remove callback based on the pointer of Executor.

In general, I like this idea. My one concern with it is the lifetime of the Executor pointer vs. the Context, but as long as we can ensure that we won't run into trouble there, I think this is a good idea.

In get_on_shutdown_callbacks, we have to make a vector based on the map.

As far as I can tell, nothing in the ROS 2 core ever calls get_on_shutdown_callbacks. Given how low-level it is, I think we could probably drop that API completely. Worst case, if we really wanted to keep it, we could just serialize the map into a vector as you suggest.

@wjwwood @ivanpauno what do you think about changing the on_shutdown_callbacks to a map and removing get_on_shutdown_callbacks?

[ivanpauno]

@wjwwood @ivanpauno what do you think about changing the on_shutdown_callbacks to a map and removing get_on_shutdown_callbacks?

SGTM

In general, I like this idea. My one concern with it is the lifetime of the Executor pointer vs. the Context, but as long as we can ensure that we won't run into trouble there, I think this is a good idea.

Executor retains shared ownership of the associated context https://github.com/ros2/rclcpp/blob/b8df9347a10bff383fdc8890c5b1bfdf16430d8f/rclcpp/include/rclcpp/executor.hpp#L542, so this should be fine.

---

I think rclcpp::spin_some() and alike should use a global executor instance instead of creating and destroying an executor each time (caveat: you cannot call rclcpp::spin_some() concurrently in that case).

[clalancette]

I think rclcpp::spin_some() and alike should use a global executor instance instead of creating and destroying an executor each time (caveat: you cannot call rclcpp::spin_some() concurrently in that case).

Certainly that is more efficient; you won't have to allocate and initialize an Executor each time it is called. Given where we are in Galactic, I'm somewhat hesitant to make a change that big. As you say, it subtly changes the semantics of rclcpp::spin_some, so it might be a surprise to existing users. And I think we can fix this particular issue without doing that, so I'll suggest we switch to the map for Galactic, and then further discuss our options for rclcpp::spin_some in H-Turtle.

[ivanpauno]

Certainly that is more efficient; you won't have to allocate and initialize an Executor each time it is called. Given where we are in Galactic, I'm somewhat hesitant to make a change that big

Yes, it's certainly not something to do for Galactic.
I only mentioned it as an improvement for the future, removing the shutdown callbacks in the destructor as suggested should fix the memory leak.

[wjwwood]

std::map on_shutdown_callbacks_;

@wjwwood @ivanpauno what do you think about changing the on_shutdown_callbacks to a map ...

I do not think it is a good idea to have Executor be known to the context...

Instead we could use get_on_shutdown_callbacks() to get the vector and remove the callback in the destructor of the executor (it returns a non-const vector).

Or we could have callback handles returned by on_shutdown, and then have a remove_on_shutdown(handle) that takes the handle to remove. We do something like this in other places.

... and removing get_on_shutdown_callbacks?

I would not do this, because then we have hidden state that once we need it, we cannot access it. I don't see any value in removing it or hiding it. 🤷‍♂️

Even if you were to store it as a map, you could still implement this function. As you guys mentioned, but it is currently a reference and a non-const one at that...

(caveat: you cannot call rclcpp::spin_some() concurrently in that case).

That's exactly why we couldn't make it global, unless it was TLS.

[wjwwood]

I would not do this, because then we have hidden state that once we need it, we cannot access it. I don't see any value in removing it or hiding it. 🤷‍♂️

If we implemented the handle system with a remove function, then I could see making get_on_shutdown_callbacks return a const reference or a copy of the vector, at least.

[ivanpauno]

I do not think it is a good idea to have Executor be known to the context...

Yes, that's a good point.

Instead we could use get_on_shutdown_callbacks() to get the vector and remove the callback in the destructor of the executor (it returns a non-const vector).

The problem is that we're storing a std::vector of std::function, and all non nullptr std::function compare non-equal.
i.e. we currently can't remove a on_shutdown_callback from the vector.

We could solve this by returning an identifier in on_shutdown_callback() (instead of returning the callback that was passed), and add a remove_shutdown_callback(IdentifierT identifier) method.
There are several ways to achieve that, e.g. we could instead store the callbacks in an unordered_set() and return an iterator to the added element.

[wjwwood]

The problem is that we're storing a std::vector of std::function, and all non nullptr std::function compare non-equal.
i.e. we currently can't remove a on_shutdown_callback from the vector.

Ah, ok, then we need the handle, something like you described or return a shared_ptr to them (not ideal) like here:

https://github.com/ros2/rclcpp/blob/b8df9347a10bff383fdc8890c5b1bfdf16430d8f/rclcpp/include/rclcpp/parameter_event_handler.hpp#L202-L203

We could also just use a static uint64_t to be a running counter, and store that value in a struct with the pointer and call that a "handle". That has the (unlikely) rollover issue though.

[wjwwood]

Also, since we cannot use the non-const vector we should make it a copy/const-ref.

[fujitatomoya]

@wjwwood @clalancette @ivanpauno thanks for the thoughts.

Ah, ok, then we need the handle

i am good to go with this.

Also, since we cannot use the non-const vector we should make it a copy/const-ref.

also agree.

@Barry-Xu-2018 would you take care of this fix?

[fujitatomoya]

related to https://github.com/ros2/rclcpp/pull/1632#discussion_r614334638

[Barry-Xu-2018]

@wjwwood @clalancette @ivanpauno @fujitatomoya

Thanks for sharing your thought.
I will make this fix.

[doisyg]

Is there an expected time for a fix to land ? Or would you advise not the use rclcpp::spin_some(node); directly for the time being and work around it ? For instance declare an executor more globaly, link it to the node and then do executor.spin_some(); locally ?

This is affecting or code in multiple places (and I bet we are not the only ones)

[wjwwood]

We're working on a fix in https://github.com/ros2/rclcpp/pull/1639. We'll land it in Rolling and Galactic I think (despite the API change). I'm not sure about backporting to other releases though.

[doisyg]

We're working on a fix in #1639. We'll land it in Rolling and Galactic I think (despite the API change). I'm not sure about backporting to other releases though.

Great!

[clalancette]

Is this fixed now that #1639 landed? Can we close this?

[wjwwood]

Yes, I think so. If anyone has time to check that this is fixed on master, that would be great, but I'm confident enough to go ahead and close this.