Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix race condition for ingesting/dispensing and disable uncrustify tests by default #362

Merged
merged 6 commits into from
Jun 8, 2024

Conversation

Yadunund
Copy link
Member

@Yadunund Yadunund commented Jun 5, 2024

#359 is blocked by uncrustify tests and #361 attempted to lint the code to fix the style checks however it became clear that having a single codebase pass uncrustify on Jammy and Noble is not feasible. Since we want to keep CI on main compatible with Humble and Rolling, the best option is to disable uncrustify checks in testing.

Signed-off-by: Yadunund <yadunund@intrinsic.ai>
@Yadunund
Copy link
Member Author

Yadunund commented Jun 5, 2024

test_rmf_task_ros2 is failling on Rolling.

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
@luca-della-vedova
Copy link
Member

I am digging at this, results so far:

This happens in Rolling but no on Jazzy.
The error is actually an rclcpp exception:

FAILED:
cannot store a negative time point in rclcpp::Time
at /usr/local/google/home/lucadv/noble_ws/src/rmf/rmf_ros2/rmf_task_ros2/test/bidding/test_SelfBid.cpp:152

I'll keep probing to see where this is done

This reverts commit 9104594.

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
@luca-della-vedova
Copy link
Member

luca-della-vedova commented Jun 7, 2024

Addressed in c2a519d (at least locally, hopefully CI doesn't prove me wrong).
It seems that using std::chrono::steady_clock::time_point::min() created the issue so I replaced min and max with "small" and "large" values, at the end of the day if I understand correctly for the estimator to work and the test to be successful it should be OK as long as one is larger than the other by some amount for QuickestFinishEvaluator to return the correct result.
Now whether this masks some other issue, that I can't tell so I'll leave it up to you @Yadunund to comment on whether you are OK with this fix or you want to dig into why we were creating negative time points when the cost was min()

@mxgrey
Copy link
Contributor

mxgrey commented Jun 7, 2024

@luca-della-vedova I was also looking into the rmf_task_ros2 failure, but you narrowed down the cause faster than me. Your fix makes perfect sense as long as the user doesn't have their system clock set to a time before the year 1970.

To be honest I find it extremely questionable that rclcpp is throwing exceptions for negative time values. Even if they want to believe that no one intends to simulate a time point before the Unix epoch, throwing an exception is an awfully blunt instrument to use for data sanitization.

@luca-della-vedova
Copy link
Member

More background on the choice is here

@luca-della-vedova
Copy link
Member

Ah well now rmf_fleet_adapter fails

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
@mxgrey
Copy link
Contributor

mxgrey commented Jun 7, 2024

It turns out there's been an extremely subtle race condition bug in the ingesting and dispensing phase implementations. I think I've seen this show up as a flaky test in the past, but it used to be so extremely rare that it didn't seem worth investigating. Something about jazzy seems to be causing the race condition to be triggered much more easily.

The logic is fixed now by 6d1e2c9. The key change is, we should always read ingestor/dispenser responses that match our request, no matter what time they are coming in relative to other messages. But we should still account for the timing of that response message when considering state updates.

The change also now allows messages with the exact same time values to be considered as a pair, whereas previously it would have forced the phase to wait for a follow-up message.

@Yadunund Yadunund changed the title Disable uncrustify tests by default Fix race condition for ingesting/dispensing and disable uncrustify tests by default Jun 7, 2024
@Yadunund
Copy link
Member Author

Yadunund commented Jun 7, 2024

@mxgrey your push did the trick ✨

@Yadunund Yadunund enabled auto-merge (squash) June 7, 2024 21:22
@Yadunund Yadunund merged commit a5d411a into main Jun 8, 2024
1 check passed
@Yadunund Yadunund deleted the yadu/remove_uncrustify_test branch June 8, 2024 02:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants