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: Fixed race condition in action server between is_ready and take_… #2248

Closed
wants to merge 1 commit into from

Conversation

jmachowinski
Copy link
Contributor

…data and execute

Some background information: is_ready and take_data are guaranteed to be called in sequence without interruption from another thread. while execute is running, another thread may also call is_ready.

The problem was, that goal_request_ready_, cancel_request_ready_, result_request_ready_ and goal_expired_ were accessed and written from is_ready and execute.

This commit fixed this by only using the mentioned variables in is_ready and take_data. execute now only accesses the given pointer and works on this.

Note, this fixes parts of #2242

Verified

This commit was signed with the committer’s verified signature.
…data and execute

Some background information: is_ready and take_data are guaranteed to be
called in sequence without interruption from another thread.
while execute is running, another thread may also call is_ready.

The problem was, that goal_request_ready_, cancel_request_ready_, result_request_ready_
and goal_expired_ were accessed and written from is_ready and execute.

This commit fixed this by only using the mentioned variables in is_ready and take_data.
execute now only accesses the given pointer and works on this.

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
@clalancette
Copy link
Contributor

Can you please target this to the rolling branch first? Our development practice is to get everything in rolling first, then backport it to stable distributions as necessary. Thanks.

@jmachowinski
Copy link
Contributor Author

I'll create a new merge request, should I leave this one just open ?

@clalancette
Copy link
Contributor

I'll close this one for now. Given the nature of this change, and the fact that we must maintain API/ABI in stable distributions, I think the final form that we would put into iron is going to be significantly different.

@jmachowinski
Copy link
Contributor Author

jmachowinski commented Jul 28, 2023

API stability would be given (I only changed private functions) , I can modify it to be also ABI compatible.

Note, we as a company are relying on this fix, and would like to see it merged ASAP. Therefore just tell me what to do, to speed the process up.

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.

None yet

2 participants