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
Install signal handlers in recorder only inside record method #1464
Install signal handlers in recorder only inside record method #1464
Conversation
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
ff476d4
to
d950c67
Compare
@tonynajjar Please review. |
I cherrypicked and tested it on iron and it works as intended 👍 I'd appreciate a backport of this to iron when merged 🙏 |
Correction, now exiting works if we are not recording but if CTRL-C is pressed while recording then the node still doesn't exit |
@tonynajjar Can you please double-check or clarify how are you reproducing failure rosbag2/rosbag2_tests/test/rosbag2_tests/test_rosbag2_record_end_to_end.cpp Lines 170 to 198 in 777612f
and record_end_to_end_exits_gracefully_on_sigterm rosbag2/rosbag2_tests/test/rosbag2_tests/test_rosbag2_record_end_to_end.cpp Lines 200 to 216 in 777612f
|
@tonynajjar Friendly ping on the question in this pull request ^^^ |
Here is an example code purely for reproduction purposes: from rosbag2_py import Recorder, RecordOptions, StorageOptions
import time
import rclpy
from rclpy.node import Node
class RosBagRecorder(Node):
def __init__(self):
super().__init__("rosbag_recorder")
storage_options = StorageOptions(
uri=f"/tmp/{time.strftime('%Y-%m-%d-%H-%M-%S')}.mcap",
storage_id="mcap",
)
record_options = RecordOptions()
record_options.all = True
recorder = Recorder()
print("created recorder")
time.sleep(3)
recorder.record(storage_options, record_options)
def main():
rclpy.init()
node = RosBagRecorder()
rclpy.spin(node)
if __name__ == "__main__":
main() I you run this node as part of a launch file and do CTRL-C while it's recording then we get:
It shows that when CTRL-C (^C), was pressed, the recording indeed stopped but not the node. The launch had to resort to sending SIGTERM after 5 seconds to shutdown the node properly which is not ideal |
@tonynajjar Thanks for providing a reproducer and sorry for the delay. |
@tonynajjar The problem is not in the rosbag2 recorder but rather in the way how you are trying to use it. But this is a wrong approach. rosbag2/rosbag2_py/src/rosbag2_py/_transport.cpp Lines 225 to 248 in d950c67
What happened in your reproducer is the following:
Please note that the composable rosbag2 recorder and player are not supported yet and this feature is still WIP Make Player and Recorder Composable (#902) #1419 |
@clalancette May I have approval for this PR? |
Hello @MichaelOrlov, can you give me a minimal example of a correct approach? Basically I need a node with a service server whose callback starts recording a bag using the Recorder. I need to spin the external node so that the service server works, how else would you do it? |
See
Just remove those |
But then the service server callback would not be called (not shown in code) |
@tonynajjar I can't comment on what is not included in the minimum valuable reproducer. With the provided minimum valuable reproducer removing |
@clalancette @fujitatomoya I would appreciate a review and approval of this PR. |
@clalancette @fujitatomoya Friendly ping for review. |
@tonynajjar just a comment. IMO, i agree that it makes sense to manage |
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.
as described in #1464 (comment), there would be more user-friendly enhancement for rosbag2_py::Recorder
, but that can be categorized as an another topic. lgtm for this fix.
Gist: https://gist.githubusercontent.com/MichaelOrlov/c860b7642218ea1655d1b603c91a9ff7/raw/42757621600bdaf3d7e3c479b7c313022320fc3a/ros2.repos |
@clalancette @emersonknapp Could you please formally approve this PR to be able to merge it? |
https://github.com/Mergifyio backport iron humble |
✅ Backports have been created
|
Signed-off-by: Michael Orlov <michael.orlov@apex.ai> (cherry picked from commit 195e406) # Conflicts: # rosbag2_py/src/rosbag2_py/_transport.cpp
Signed-off-by: Michael Orlov <michael.orlov@apex.ai> (cherry picked from commit 195e406)
…d (backport #1464) (#1526) * Install signal handlers in recorder only inside record method (#1464) Signed-off-by: Michael Orlov <michael.orlov@apex.ai> (cherry picked from commit 195e406) # Conflicts: # rosbag2_py/src/rosbag2_py/_transport.cpp * Address merge conflicts Signed-off-by: Michael Orlov <michael.orlov@apex.ai> --------- Signed-off-by: Michael Orlov <michael.orlov@apex.ai> Co-authored-by: Michael Orlov <michael.orlov@apex.ai>
Notes for reviewers:
Recorder::record(..)
method for instance handling some service events before initiating recording. During those times signal handlers shall not be overridden.rclcpp::SignalHandlerOptions::None
andshutdown_on_signal
options since it doesn't have any effect because we are replacing signals with our own signal handlers even if they we installed before or if context has already been initialized before.