[foxy backport] Derive and throw exception in spin_some spin_all for StaticSingleThreadedExecutor (#1220)

[brawner]

This backports just changes to the headers, not the unit test.

Signed-off-by: Stephen Brawner brawner@gmail.com

[brawner]

• Linux
• Linux-aarch64
• macOS
• Windows

[jacobperron]

@brawner I think you need to also provide the foxy repos file for CI.

[jacobperron]

I wasn't sure if this change was ABI compatible so I ran an ABI checker and it looks good 👍

[brawner]

Rerunning tests.

I wasn't sure if this change was ABI compatible so I ran an ABI checker and it looks good

Thanks! But I think that may because those implementations are header only. If the implementation eventually gets moved to the source file, do we have anything to worry about?

Also, this file wasn't actually being tested anywhere and not linked to. Would the ABI checker recognize it?

[jacobperron]

I don't know the answer to either of your questions 😅

Maybe someone from @ros2/team can weigh in?

[brawner]

From what I can tell from: https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B under section "Adding a reimplemented virtual function"

This will break ABI compatibility if anyone calls the derived function explicitly:

rclcpp::executors::StaticSingleThreadedExecutor executor;
executor.add_node(node);
executor.spin_some();

But wouldn't if they called the base class implementation:

rclcpp::Executor executor = rclcpp::executors::StaticSingleThreadedExecutor();
executor.add_node(node);
executor.spin_some();

Since the compiler will not include the vtable lookup in the first version.

As these functions do not currently work at all, this might be a potentially necessary breaking ABI change that can helps users understand why it broke.

[clalancette]

Right, as far as I can tell, this will break ABI, because it will change the offsets into the derived object (as you correctly state above).

I'm on the fence about whether it is worth it to break ABI for this. On the "yes, let's break ABI" side, Foxy is only a month old, and so the number of users is probably fairly low (and thus the scope of the breakage is low). Further, there probably aren't very many users of StaticSingleThreadedExecutor. On the "no, let's not break ABI" side, the bar to break ABI is pretty high, and I'm not sure that on its own, this clears it. The problem doesn't seem severe enough to me to potentially break users (which, if they don't recompile, will break in incomprehensible ways too).

I'd lean towards "no, let's not break ABI, and just document the problem", but I could be persuaded otherwise. I'd like to hear more opinions.

[chapulina]

I often refer to this guide from KDE when it comes to ABI. They have some tips to workaround adding a reimplemented virtual function, which could be done for Foxy, then the correct thing can be done for Galactic.

I'd lean towards "no, let's not break ABI

+1

[jacobperron]

I'm also leaning towards not breaking API and documenting the issue. We should add it to the list of known issues for Foxy.

[brawner]

For reference, this is the open rclcpp issue. #1219

I'll make a PR for submitting it to known issues.

[brawner]

Closing in favor of #1385