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

Add wait_for_shutdown utility #29

Merged
merged 1 commit into from
Oct 18, 2023

Conversation

mhidalgo-bdai
Copy link
Contributor

@mhidalgo-bdai mhidalgo-bdai commented Oct 2, 2023

This patch brings a wait_for_shutdown utility to wait for a given context to shutdown. Useful to block the main thread while callbacks are invoked in the background. Builds on #32.

@mhidalgo-bdai mhidalgo-bdai changed the title Add wait_for_shutdown utility [DO NOT REVIEW] Add wait_for_shutdown utility Oct 2, 2023
@mhidalgo-bdai mhidalgo-bdai changed the title [DO NOT REVIEW] Add wait_for_shutdown utility Add wait_for_shutdown utility Oct 3, 2023
Copy link
Collaborator

@bhung-bdai bhung-bdai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar question to the other threaded PRs: does this shut down only the work being done on that thread, or does it shut down the executor globally?

@mhidalgo-bdai
Copy link
Contributor Author

does this shut down only the work being done on that thread, or does it shut down the executor globally?

wait_for_shutdown does not shut down anything, it only performs a blocking wait until shutdown or timeout. As it's blocking, shutdown can only happen from another thread or signal handler. It serves the same purpose as rospy.spin did: just hold until ROS 2 shuts down. It's convenient in the main thread when the executor is spinning in the background.

On a second thought, one could also disable background spinning and spin explicitly in the main thread. I'll add that option too.

Copy link

@ksharma-bdai ksharma-bdai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wait_fot_shutdown portion looks straightforward and convenient, will approve after #32 is merged and this is rebased.

Signed-off-by: Michel Hidalgo <[email protected]>
Copy link

@ksharma-bdai ksharma-bdai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, essentially a 1:1 breakout of the prior code into a useful helper.

@ksharma-bdai ksharma-bdai merged commit a1273c7 into bdaiinstitute:main Oct 18, 2023
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.

3 participants