Feature: expose event callback setter in subscription, service, client and timer

[nadavelkabets]

Part of #1399.

• Expose set_on_new_message_callback and clear_on_new_message_callback for subscription.
• Expose set_on_new_request_callback and clear_on_new_request_callback for service.
• Expose set_on_new_response_callback and clear_on_new_response_callback for client.
• Expose set_on_reset_callback and clear_on_reset_callback for timer.

[nadavelkabets]

@sloretz @fujitatomoya @mjcarroll @wjwwood @ahcorde
The PR is complete with unit tests.
The python callback provided by the user is wrapped by pybind and gets executed in the rcl event trampoline.
Any exception raised in the python callback might result in a core dump since the event trampoline executes in a pure c environment.
To prevent core dumps, I wrapped user callbacks in the asyncio executor with an exception handler that logs exceptions.
I'm not sure if that's a big issue as this code is not exposed to the end user and should only be accessed by an executor implementation.
What do you think?

[nadavelkabets]

@mjcarroll @wjwwood @skyegalaxy
Following our discussion in the meeting group, I added exception handling in the rcl trampoline that prints any exception and exits to avoid propagation into c code.

[nadavelkabets]

@fujitatomoya do you have the time to take a look at this PR?

[skyegalaxy]

looking at the PR just from a C++ and python perspective, everything looks alright to me. I'll defer to others with more rclpy experience as I've mostly spent time on the rclcpp side. I appreciate that the code will now bubble up the exceptions and exit instead of silently failing!

[wjwwood]

lgtm too, with a small non-blocking suggestion

[nadavelkabets]

Would be great to run CI on this PR.
Is anybody available?

[fujitatomoya]

Pulls: #1496
Gist: https://gist.githubusercontent.com/fujitatomoya/7983ca49fce3c3d9e3fd1bc3da1b814f/raw/323fcc8714382080808b8734f120667ba00b692e/ros2.repos
BUILD args: --packages-above-and-dependencies rclpy
TEST args: --packages-above rclpy
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/16942

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[fujitatomoya]

@wjwwood thanks for reviewing this 🚀

@nadavelkabets CI is just started.

[nadavelkabets]

@fujitatomoya I made the following fixes:
diff link: e2452bb

• Add docstring in .pyi
• Call std::terminate instead of std::exit upon unhandled exception
• Improve the exception string when throwing RCLError
• clear any existing callback when destroying an entity

Could you please run CI again?

[fujitatomoya]

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[fujitatomoya]

@nadavelkabets all gree, i will go ahead to merge this. thank you very much for your contribution.

[nadavelkabets]

@fujitatomoya Could we backport this as well?

[fujitatomoya]

@Mergifyio backport kilted jazzy

[mergify]

backport kilted jazzy

✅ Backports have been created

Details

• #1502 Feature: expose event callback setter in subscription, service, client and timer (backport #1496) has been created for branch kilted but encountered conflicts
• #1503 Feature: expose event callback setter in subscription, service, client and timer (backport #1496) has been created for branch jazzy but encountered conflicts

[fujitatomoya]

@nadavelkabets we can backport this, so i requested kilted and jazzy for now. i did not create humble one, because the code base is really different.