Cache disable flag to avoid reading environmental variable.

[fujitatomoya]

address #977

Signed-off-by: Tomoya Fujita Tomoya.Fujita@sony.com

[fujitatomoya]

@clalancette minor performance improvement. this can avoid reading environmental variable every time it publishes data with using LoanedMessage.

CC: @MichaelOrlov would be interested for you, as performance improvement.

[fujitatomoya]

@clalancette requesting 2nd review, modification for test code is bigger than fix itself...

[fujitatomoya]

can we backport this humble? probably not, since c structure is extended, this is API compatibility but ABI?

[fujitatomoya]

https://build.ros2.org/job/Rpr__rcl__ubuntu_jammy_amd64/154/ is related to this PR, but cannot reproduce this failure on my local environment.

@ros-pull-request-builder retest this please

[fujitatomoya]

either @Barry-Xu-2018 or @iuhilnehc-ynos can you run the test with your local environment? I cannot reproduce the failure reported https://build.ros2.org/job/Rpr__rcl__ubuntu_jammy_amd64/159/.

colcon test --event-handlers console_direct+ --packages-select rcl

[iuhilnehc-ynos]

@fujitatomoya

I can reproduce this issue in the ros:rolling image with this PR. The failure in Rpc is because the rmw_fastrtps is not the latest.
After using rmw_fastrtps with at least ros2/rmw_fastrtps@19d141d, the failure was gone.

It means the ros-rolling-rmw-fastrtps-cpp should be updated in http://repo.ros2.org/ubuntu/testing.

[fujitatomoya]

It means the ros-rolling-rmw-fastrtps-cpp should be updated in http://repo.ros2.org/ubuntu/testing.

Ah, okay so using latest repo for rolling and CI should be fine. i will start the CI to verify. @iuhilnehc-ynos thanks for the information.

@clalancette i thought Rpr job base runtime is updated nightly, but obviously it is not. How often does it get updated?

[clalancette]

@clalancette i thought Rpr job base runtime is updated nightly, but obviously it is not. How often does it get updated?

Only when we do releases into Rolling. We are way overdue for that; we will spend some time next week doing releases into Rolling.

[fujitatomoya]

CI:

• Linux
• Linux-aarch64
• Windows

[fujitatomoya]

Window is unstable, i will restart it later.

[fujitatomoya]

• Windows

[MichaelOrlov]

@fujitatomoya Thanks for moving to rcl_options and checking environment variable inside get_default_options methods. Implementation looks good to me.

However I have a question and suggestion for unit tests.

• Need to check that explicitly settled up value for the publisher_options.disable_loaned_message and subscriber_options.disable_loaned_message will not be overridden by the environment variable after calling pub/sub init.
• And we shouldn't rely on default value of the environment variable. We should set environment variable explicitly for each test.

[fujitatomoya]

CI:

• Linux
• Linux-aarch64
• Windows

[fujitatomoya]

@iuhilnehc-ynos can you review this?

[MichaelOrlov]

@fujitatomoya Thanks for addressing comments from review.
I still have a strong opinion that we should not rely on default settings for ROS_DISABLE_LOANED_MESSAGES environment variable in test_publisher_loan_disable and test_subscription_loan_disable when doing check for EXPECT_FALSE(publisher_options.disable_loaned_message); right after reading default options.
We need either explicitly set environment variable in test or even better remove check for EXPECT_FALSE(publisher_options.disable_loaned_message); after getting default options. And always forcefully set publisher_options.disable_loaned_message value as we needed for test case.
The rationale for that is:

1. It's not safe to rely (not reliable to assume) that ROS_DISABLE_LOANED_MESSAGES environment variable will not be set by default on testing machine and environment. It could be leftovers from another tests. It might work currently but could be broken in future if order of tests will be changed or some new tests would be added.
2. You've already have dedicated tests for checking publisher_options.disable_loaned_message after getting default options with different values of the ROS_DISABLE_LOANED_MESSAGES environment variable.

[iuhilnehc-ynos]

LGTM with some minor comments.

[fujitatomoya]

@fujitatomoya Thanks for addressing comments from review. I still have a strong opinion that we should not rely on default settings for ROS_DISABLE_LOANED_MESSAGES environment variable in test_publisher_loan_disable and test_subscription_loan_disable when doing check for EXPECT_FALSE(publisher_options.disable_loaned_message); right after reading default options. We need either explicitly set environment variable in test or even better remove check for EXPECT_FALSE(publisher_options.disable_loaned_message); after getting default options. And always forcefully set publisher_options.disable_loaned_message value as we needed for test case. The rationale for that is:

1. It's not safe to rely (not reliable to assume) that ROS_DISABLE_LOANED_MESSAGES environment variable will not be set by default on testing machine and environment. It could be leftovers from another tests. It might work currently but could be broken in future if order of tests will be changed or some new tests would be added.
2. You've already have dedicated tests for checking publisher_options.disable_loaned_message after getting default options with different values of the ROS_DISABLE_LOANED_MESSAGES environment variable.

@MichaelOrlov I see, probably it would be better, not to have flaky test 👍

[fujitatomoya]

CI:

• Linux
• Linux-aarch64
• Windows

[fujitatomoya]

@MichaelOrlov @iuhilnehc-ynos thanks for the review, all comments are addressed. requesting final check.

[fujitatomoya]

@iuhilnehc-ynos thanks for the review.

CI(windows unstable):

• Windows

[MichaelOrlov]

@fujitatomoya Please remove logging to the stderr stream when rcl_get_disable_loaned_message(&disable_loaned_message) returning failure.
This is a normal situation when ROS_DISABLE_LOANED_MESSAGES is not defined and ret != RCL_RET_OK in this particular situation.

[fujitatomoya]

@clalancette i am putting a hold on this until #1029 (comment) is resolved. after rolling release, i will kick the Rpr job to make sure it passes, and then i will merge this into rolling.

[fujitatomoya]

@ros-pull-request-builder retest this please

[fujitatomoya]

rpr job is happy now, CI is green.

@clalancette i will be waiting for the final review on this.

[fujitatomoya]

@clalancette @MichaelOrlov @iuhilnehc-ynos thank you for the review, i will go ahead to merge this. since this changes ABI, this will be available on Iron or later.