Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix for timer/srv-server finalization issue 2283 #2284

Open
wants to merge 4 commits into
base: noetic-devel
Choose a base branch
from

Conversation

aurzenligl
Copy link

@aurzenligl aurzenligl commented Sep 28, 2022

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:

FYI: @iwanders, @jacobperron, @fujitatomoya

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.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant