Fix for timer/srv-server finalization issue 2283 #2284
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
Making sure currently executing callback finishes before removing thread returns from removeByID allows to avoid race condition in a case when e.g. ros::Timer held in a class and capturing `this` is stopped just before destruction of the class and its data members: https://github.com/aurzenligl/study/blob/master/ros-timer/src/race.cpp
Rationale for the testcase was the following deadlock scenario: https://gist.github.com/iwanders/ede48fb649fd47f9b1f9a52c527b463c Changed testcase presents how the same scenario can be carried out with blocking removeByID (with exception for self-removal). The external mutex from the scenario must be unlocked for the call of ros::Timer::stop, otherwise scenario stays as it is. External thread returns from removeByID once cb call finishes, spinner thread returns immediately.
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.
As explained in #2283, delivering #2121 opened a possibility for a race condition during ros::Timer and ros::ServiceServer finalization: https://github.com/aurzenligl/study/blob/master/ros-timer/src/race.cpp
In order to remedy the situation, but still allow the scenario mentioned in #2121 to succeed, I propose to define post-condition of removeById this way: callback is not and will not be executed, unless removal happened in the context of callback (self-removal). It's compatible with user code prior to #2121, as then removeByID always blocked (even on self-removal, leading to deadlock).
This allows to satisfy both use cases:
doesn't deadlock as long as timer stop is called w/o external mtx locked
(see: selfRemoveCallbackWhileExecuting testcase),
doesn't crash as ros::Timer::stop guarantees no spinner uses any cb-captured data anymore
(see: removeCallbackWhileExecuting testcase).
FYI: @iwanders, @jacobperron, @fujitatomoya