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

Timers will cancel on shutdown #181

Merged
merged 4 commits into from
Feb 15, 2019
Merged

Timers will cancel on shutdown #181

merged 4 commits into from
Feb 15, 2019

Conversation

pbaughman
Copy link
Contributor

@pbaughman pbaughman commented Feb 12, 2019

  • By default, a shutdown event will cancel a TimerAction
  • The timers used to send SIGKILL and SIGTERM to processes don't cancel on shutdown

From issue #180

The only TimerActions I found in launch are the sigterm timer and sigkill timer in launch/actions/execute_process.py. I've set these up to not cancel on shutdown

@wjwwood I'm a little concerned about the thread safety note here. I didn't deep dive the thread model for event EventHandler actions. Do I need to be more cautious about calling self.cancel from an OpaqueFunction in TimerAction.execute?

Todo:

  • Make sure this works for ctrl+c, otherwise it's weird

@tfoote tfoote added the in review Waiting for review (Kanban column) label Feb 12, 2019
@pbaughman pbaughman closed this Feb 12, 2019
@tfoote tfoote removed the in review Waiting for review (Kanban column) label Feb 12, 2019
@pbaughman pbaughman reopened this Feb 12, 2019
@tfoote tfoote added the in review Waiting for review (Kanban column) label Feb 12, 2019
@pbaughman
Copy link
Contributor Author

I have tests for this that I can push, but they need #184 to be merged first

@wjwwood wjwwood changed the title ISSUE180 Timers will cancel on shutdown Timers will cancel on shutdown Feb 14, 2019
@wjwwood
Copy link
Member

wjwwood commented Feb 14, 2019

Looks like it needs to be rebased or merged now (I prefer rebase, but up to you).

@wjwwood
Copy link
Member

wjwwood commented Feb 14, 2019

The only TimerActions I found in launch are the sigterm timer and sigkill timer in launch/actions/execute_process.py. I've set these up to not cancel on shutdown

👍

@wjwwood I'm a little concerned about the thread safety note here. I didn't deep dive the thread model for event EventHandler actions. Do I need to be more cautious about calling self.cancel from an OpaqueFunction in TimerAction.execute?

It's fine, all actions are executed in the asyncio run loop, and so are all event handlers, so it's all single threaded.

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm

@wjwwood wjwwood mentioned this pull request Feb 14, 2019
@pbaughman
Copy link
Contributor Author

pbaughman commented Feb 14, 2019 via email

Pete Baughman added 2 commits February 14, 2019 09:29
 - By default, a shutdown event will cancel timers
 - The timers used to send SIGKILL and SIGTERM to processes don't cancel on shutdown
  - Check that timers will shut down a launched system
  - Check that shutdown will cancel a timer without cancel_on_shutdown=False
  - Check that shutdown will not cancel a timre with cancel_on_shutdown=True
@pbaughman
Copy link
Contributor Author

@wjwwood This should be good to go. Some new tests added for the new timer behavior

@wjwwood
Copy link
Member

wjwwood commented Feb 15, 2019

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@wjwwood
Copy link
Member

wjwwood commented Feb 15, 2019

CI after fixing quotes:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood
Copy link
Member

wjwwood commented Feb 15, 2019

CI (one more time):

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@wjwwood wjwwood merged commit e870424 into ros2:master Feb 15, 2019
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Feb 15, 2019
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