Fix bug that a callback not reached

[iuhilnehc-ynos]

a subscriber on a new executor with a callback group triggered to receive a message

related to #1611

Signed-off-by: Chen Lihui [email protected]

[iuhilnehc-ynos]

this test case expects test result as

The following tests FAILED:
	 26 - test_add_callback_groups_to_executor__rmw_fastrtps_cpp (Failed)
	 27 - test_add_callback_groups_to_executor__rmw_fastrtps_dynamic_cpp (Failed)

after using ros2/rmw_fastrtps#527, there is no failure.

[asorbini]

Before merging this test, can we discuss whether attaching a node to two different executors should be allowed by the API? This isn't currently a requirement AFAIK, so making this change is not trivial at all.

[wjwwood]

I reexamined the test to check what @asorbini was asking about, and I think I found some flaws, but the general idea (i.e. using more than one executor per node via add_callback_group() and having a callback group wake an executor when something is added to it in order to consider new items) is a supported use case based on the API.

It might be that this is a bug in rclcpp and not the middlewares however. I need to look more deeply at the root cause of the issue. But I don't have time at the moment.

[iuhilnehc-ynos]

This kind of test source code seems not good (adding it into rclcpp might make other users wrongly use it in the future), close it.

[fujitatomoya]

i do not know why we need to close this. after rclcpp is fixed, we can add this kind of test?

[wjwwood]

i do not know why we need to close this. after rclcpp is fixed, we can add this kind of test?

I don't think we needed to close it, but I do think the things I pointed out needed to be addressed.

I do not think, however, that we should have tests that copy user submitted code by including anti-patterns or otherwise not-recommended practices. Instead, the test should (ideally) test one potential issue at a time. It can be distilled from a user reported issue, but again it should be cleaned up and written as properly as possible.

[fujitatomoya]

yeah okay that makes sense to me.

[iuhilnehc-ynos]

reopen it after not using some anti-pattern source code.

[iuhilnehc-ynos]

@wjwwood

I have updated rclcpp source code by following #1611 (comment) if I understood correctly.
Could you help to review it again?

[fujitatomoya]

overall, i think it looks good.

[fujitatomoya]

@wjwwood friendly ping.

[SteveMacenski]

Noting that this is causing a bug in the Humble migration and this fixes it for an issue in Nav2 linked above @clalancette

Obviously too late in the game for this to get into Humble out of the box, but hope that it will get in for the LTS and backported. The issues we ran into were very subtle.

[clalancette]

It looks like this needs a rebase, and another review for the latest changes. Once that is done, we can hopefully merge it in for Rolling.

Backporting it to Humble is going to be tricky, as it changes ABI. We'll have to consider another way to do that.

[iuhilnehc-ynos]

@wjwwood friendly ping.

[clalancette]

I rebased this PR and made a few minor changes (just to the include files).

Overall, I think the idea behind this PR is good, but I will point out that it is not thread-safe. In particular, the lock around get_notify_guard_condition is useless; it is only held while manipulating the shared_ptr, but is then dropped when we return the object. Anything we do with that object afterwards is not thread-safe.

What I think we should do instead is to make sure we do any operations on the callback-group guard condition with the lock held. That would require us to add some additional APIs (like a new trigger method), modify some of the existing ones (change destroy_notify_guard_condition to only destroy the guard condition if it was created and not error out), and otherwise clean some things up. With those things done, we should be able to make this thread-safe and move forward with this.

[iuhilnehc-ynos]

One callback group can only have one notify_guard_condition_, the notify_guard_condition_mutex_ is jsut for these three operations, and the callback group can be used in only one executor.

so it seems unnecessary to add extra mutex to make the callback-group guard condition thread-safe.

[iuhilnehc-ynos]

CI:

• Linux
• Linux-aarch64
• Windows

[iuhilnehc-ynos]

retry CI:

• Windows

[fujitatomoya]

@iuhilnehc-ynos thanks for the effort on this, i will take a look at this today. I think this is very helpful for user application.

[fujitatomoya]

lgtm

[fujitatomoya]

dependent and above CI all green, i will go ahead to merge this.

[fujitatomoya]

@iuhilnehc-ynos i think we can backport this to humble, right? it would be really nice to backport since this affects user application behavior.

[fujitatomoya]

@mergify backport this to humble

Just create backport PR so that we will not forget, let's give it a couple of weeks to make sure there is no degression, then we can merge the backport PR to humble.

[mergify]

backport this to humble

❌ No backport have been created

• Backport to branch this failed: Branch not found
• Backport to branch to failed: Branch not found
• #2033 Fix bug that a callback not reached (backport #1640) has been created for branch humble