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 that lead to miss first message #1054

Closed
wants to merge 3 commits into from

Conversation

kamibo
Copy link

@kamibo kamibo commented May 11, 2017

Callback queue waits for callback from "callOne" method.
When internal queue is not empty this last method succeeded even if id
info mapping does not contains related callback's id.
In this case, first callback (for one id) is never called since
"addCallback" method first push callback into internal queue and then
set info mapping.

So id info mapping has to be set before push callback into interal
queue. Otherwise first message could be ignored.

Attached file highlights this issue.
ros_callback_queue_bug.cpp.zip

kamibo and others added 2 commits May 11, 2017 17:27
Callback queue waits for callback from "callOne" method.
When internal queue is not empty this last method succeeded even if id
info mapping does not contains related callback's id.
In this case, first callback (for one id) is never called since
"addCallback" method first push callback into internal queue and *then*
set info mapping.

So id info mapping has to be set before push callback into internal
queue. Otherwise first message could be ignored.
@esteve
Copy link
Member

esteve commented May 17, 2017

@kamibo I've pushed kamibo#1 which includes your fix and a test for the race condition. I've removed the C++11 and C++14 parts from the test and replaced them with Boost so it builds fine on a strict Indigo.

@dirk-thomas
Copy link
Member

@kamibo Can you please incorporate the test provided by @esteve and target the latest development branch with this PR (lunar-devel).

@esteve
Copy link
Member

esteve commented May 18, 2017

@kamibo I've created #1058 which is just a rebase of this branch on top of lunar-devel

@dirk-thomas do you need a PR for Kinetic as well or you'll back/forward port it yourself?

@kamibo
Copy link
Author

kamibo commented May 18, 2017

Thanks @esteve it's perfect !

Added test for addCallback race condition
@dirk-thomas
Copy link
Member

Closing this in favor of #1058. The patch will be considered for backporting into older distros from the lunar-devel branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants