Segfault while detaching service client in `rmw_wait`

[dsobek]

Generated by Generative AI

No response

Operating System:

Jetson NX Orin and Jetson AGX Orin

ROS version or commit hash:

humble

RMW implementation (if applicable):

rmw_cyclonedds_cpp

RMW Configuration (if applicable):

No response

Client library (if applicable):

rclcpp

'ros2 doctor --report' output

ros2 doctor --report

<COPY OUTPUT HERE>

Steps to reproduce issue

I have not been able to create a sharable reproduction case yet. We've only observed this segfault occurring on a few pieces of hardware, and very rarely.

However see details below; I've done a decent amount of debugging and maybe someone more knowledgeable than I will be able to construct an example.

Expected behavior

My application runs without crashing

Actual behavior

Seg fault akin to #460 (comment).

This segfault occurs specifically when destroying a rclcpp service client (I haven't observed this with action clients yet).

The segfault occurs on this line:

rmw_cyclonedds/rmw_cyclonedds_cpp/src/rmw_node.cpp

Line 3958 in 7cb3b38

dds_waitset_detach(ws->waitseth, x->client.sub->rdcondh);

Through debugging with gdb, x->client.sub comes up as inaccessible memory, I've also been able to confirm that the client is destroyed just before the segfault occurs.

Application details

We are using a rclcpp multithreaded executor and creating clients on a thread that is not managed by the executor. The client is also being constructed with the default callback group (which I believe is a mutually exclusive callback group). There are a lot of entities being added onto the wait set, and not just service clients.

My Theory

I believe I'm seeing a race condition between different places where we are detaching or reattaching entities in the wait set, depending on when rclcpp destroys the client.

rclcpp's memory strategy concept is used to make sure that different dds entities at the rclcpp layer (so clients, topic publishers/subsribers, etc.) only really ensures that these entities stay alive while rmw_wait is called. Since these entities are are shared pointers at this layer, and the memory strategy only temporarily takes shared ownership via weak pointers when calling rmw_wait, there are two places the client can be destroyed:

1. when the executor clears and recollects entities just before rmw_wait
2. Or in whichever thread owns the shared pointer returned by create_client.

In the first case, no segfault should occur, the client/entity is being destroyed explicitly while rmw_wait is not being invoked (the multithreaded executor holds a mutex lock to ensure this). However the second case could occur at any time.

Ultimately, this race condition is really rare, and I think this segfault only occurs under the following series of events in the rmw layer (under the previously stated conditions in the rclcpp layer):

Thread 1: rmw_wait invoked, wait set is marked as "inuse" (code)
Thread 2: client destruction started, globally cached waitsets are detached (except for the one that is "inuse", i.e. the wait set being used by thread 1)  (code)
Thread 2: client handle is deleted (code)
Thread 1: rmw_wait reattaches entities due to the wait set's cache being outdated, detaching the old cached entities (including the just deleted one, causing the segfault) (code)

Additional information

Attempted fixes

The waitset has a mutex protecting the inuse varaible. Instead of skipping the "inuse" waitset when cleaning waitset caches I attempted to block on this waitset until it was no longer in use. This caused a deadlock for me unfortunately.

I also tried to get rid of the case where the client is destroyed in a separate thread by holding the shared pointer for longer in the memory strategy, but this is difficult to do without causing a memory leak: If done wrong, the shared pointers can be held forever and the clients will never be freed.

[eboasson]

Thank you @dsobek! I remember #460 rather well, and your analysis finally explains it. I probably wouldn't ever have figured this out, as I'm not sufficiently familiar with the ownership/memory strategy in the upper layers ...

There's one obvious solution, and that's to remove the caching. I added that caching because it helped reduce the cost of bridging the gap between the way the RMW and the DDS interfaces support waiting for an event (RMW is like select, DDS is like epoll and hence with completely different assumptions about the relative costs of operations). In the simple test programs I had it made a significant different, but I don't know if that would still be the case or if it holds beyond simple programs. Perhaps it is not worth it (anymore).

[MichaEiler]

I wonder whether I end up in a similar situation. Just with ROS2 jazzy in my case. It points me to the following line: rmw_node.cpp#L4269 within the waitset_detach method.

I'm using several lifecycle nodes which upon cleaning them up unregister/destroy other services within those node. The cleanup transition is called from another executor through a change_state service call.

I can reproduce it currently through running a unit test repeatedly and sometimes depending on cpu load ending up in this state.

Thread 1:
Executing the change_state service call and likely waiting for the result: Stacktrace 1

Thread 2:
Segfaulting in waitset_detach: Stacktrace 2

In contrast to the bug-report above the questions is of course also whether what I try to do is valid in first place.

[dsobek]

@eboasson I'm not very helpful with the event handling stuff at the moment, but I can certainly test and benchmark my application without the caching if you have some guidance on how to do that.

Also, if keeping the caching in place, do you think it would be possible to delay destruction of entities to the end of the rmw_wait call for entities that are in an "inuse" waitset? My thinking is that there'd be global state similar to, or within, gcdds() that could hold these entities until the waitset is no longer in use. Just an idea I'm workshopping; it would add more complexity and could just make different race conditions.