[eloquent] Ensure rcl_clock accesses are thread-safe (ABI-safe version).

[clalancette]

This is an alternative PR to #999
The code in this one is less nice, but it is ABI-safe. Thus,
it can be used as a short-term workaround for users hurt by
the problem now, and/or used as the solution for Eloquent.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

I'm still testing this, so consider this a request for comment on the approach. Once I run a full CI on it, I'll be more confident in it.

[clalancette]

CI on Linux (all packages, all tests, Eloquent/bionic):

[fujitatomoya]

LGTM

[wjwwood]

lgtm, with one issue raised.

[clalancette]

CI on Linux (all packages, all tests, Eloquent/bionic):

For comparison, a build before this PR (all packages, all tests, Eloquent/bionic):

[clalancette]

@ros-pull-request-builder retest this please

[hidmic]

LGTM and CI's happy. I think this one's ready to go.

[clalancette]

@mjcarroll correctly pointed out that https://ci.ros2.org/job/ci_linux/10124/testReport/junit/(root)/test_rclcpp/gtest_spin__rmw_connext_cpp_gtest_missing_result/ looks suspiciously like a regression from this patch. I wasn't able to reproduce locally, but I think it needs more looking into.

[clalancette]

After doing a bit of debugging here, it turns out that there are some accesses to the TimerBase object that don't go through the Clock constructor, so they never get a new entry in the global mutex map assigned. I'm not sure how that can happen (the TimerBase constructor takes a rclcpp::Clock::SharedPtr), but it seems to be the cause of the crash we are seeing in CI. Needs more investigation.

[clalancette]

Thanks to some help from @hidmic, I think we've solved the crash. Here is another CI on Linux (all packages, all tests, Eloquent/Bionic), to be sure:

[clalancette]

There are 194 failures in the Linux build. If I filter out all of the flake8 and pep257 ones, that leaves me with the following list:

    projectroot.test_publisher_subscriber__rclpy__rmw_connext_cpp__rmw_cyclonedds_cpp
    projectroot.test_publisher_subscriber__rclpy__rclcpp__rmw_connext_cpp__rmw_cyclonedds_cpp
    projectroot.test_publisher_subscriber__rclcpp__rclpy__rmw_connext_cpp__rmw_cyclonedds_cpp
    projectroot.test_publisher_subscriber__rclcpp__rmw_connext_cpp__rmw_cyclonedds_cpp
    projectroot.test_events__rmw_cyclonedds_cpp
    rcl.TestEventFixture__rmw_cyclonedds_cpp.test_pubsub_deadline_missed
    rcl.TestEventFixture__rmw_cyclonedds_cpp.test_pubsub_liveliness_kill_pub
    test_communication.TestPublisherSubscriberAfterShutdown.test_processes_finished_gracefully[WStrings]
    test_communication.TestPublisherSubscriberAfterShutdown.test_processes_finished_gracefully[WStrings]
    test_communication.TestPublisherSubscriberAfterShutdown.test_processes_finished_gracefully[WStrings]
    test_communication.TestPublisherSubscriberAfterShutdown.test_processes_finished_gracefully[WStrings]

I believe that the WString interoperability with Connext is a known issue, so we can ignore those. That leaves us with:

    rcl.TestEventFixture__rmw_cyclonedds_cpp.test_pubsub_deadline_missed
    rcl.TestEventFixture__rmw_cyclonedds_cpp.test_pubsub_liveliness_kill_pub

But if memory serves correctly, the Cyclone version in Eloquent didn't support liveliness or deadline. So overall, I think we're pretty good to go here. I'm going to run one last CI on the other platforms, then go ahead and merge this.

[clalancette]

• Linux-aarch64
• macOS
• Windows

[clalancette]

Windows "failed" because it had too many test failures. But I honestly don't know what I should expect for failures there. Here's a Windows CI run without this patch:

• Windows

[clalancette]

While we could potentially slip this into Eloquent, I think it is too risky given that Eloquent is going EOL in 4-6 weeks. I'm just going to close this out; the fix is in Foxy and later.