fix: Fixed race condition in action server between is_ready and take (humble) #2635
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… Backport from iron ros2#2531
This was referenced Sep 30, 2024
jmachowinski
suggested changes
Oct 1, 2024
| size_t next_ready_event = pimpl_->next_ready_event.exchange(ClientBaseImpl::NO_EVENT_READY); | ||
|
|
||
| if (next_ready_event == ClientBaseImpl::NO_EVENT_READY) { | ||
| // there is a known bug in iron, that take_data might be called multiple |
There was a problem hiding this comment.
This bug should not be present in Humble, therefore in the humble version there should be a throw, to avoid covering other bugs.
| if (!data) { | ||
| throw std::runtime_error("'data' is empty"); | ||
| if (!data_in) { | ||
| // workaround, if take_data was called multiple timed, it returns a nullptr |
There was a problem hiding this comment.
Same here, if a null pointer gets passed to execute, we should throw.
| ret, result_request, request_header)); | ||
| } else if (pimpl_->goal_expired_.load()) { | ||
| if (next_ready_event == ServerBaseImpl::NO_EVENT_READY) { | ||
| // there is a known bug in iron, that take_data might be called multiple |
| } else { | ||
| throw std::runtime_error("Executing action server but nothing is ready"); | ||
| if (!data_in) { | ||
| // workaround, if take_data was called multiple timed, it returns a nullptr |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related bug: #2242. I managed to create a situation in which I can easily replicate it as many times as I wanted.
This bug was already fixed in rolling (#2495) and iron (#2531).
I just backported the fix based on the iron fix. Now I test it and works perfectly all the times I run my node.
I tried to do it as clean as possible (fortunately the iron fix didn't touch header files and all the function prototypes where the same between iron and humble).