-
Notifications
You must be signed in to change notification settings - Fork 417
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: Fixed race condition in action server between is_ready and take (humble) #2635
base: humble
Are you sure you want to change the base?
fix: Fixed race condition in action server between is_ready and take (humble) #2635
Conversation
… Backport from iron ros2#2531
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replace by throw
} 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replace by throw
This also needs DCO sign-off. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edgarcamilocamacho friendly ping
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).