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

fix: Handle SIGTERM more gracefully in watchmedo #693

Merged
merged 2 commits into from Oct 22, 2020

Conversation

maybe-sybr
Copy link
Contributor

This fixes some misbehaviour where the child process could be orphaned
if watchmedo received multiple SIGTERMs causing the stop() call to be
interrupted. By raising a semantic exception after neutering the signal
handler, we give ourselves a better chance of killing the child.

@BoboTiG
Copy link
Collaborator

BoboTiG commented Oct 20, 2020

Hello @maybe-sybr,

Thanks for the patch 💪
Would that fix #543?

@maybe-sybr
Copy link
Contributor Author

It sounds very similar but I think my change would only fix the behaviour for repeated SIGTERMs and repeated SIGINTs would still misbehave, The KeyboardInterrupt which would get raised by a SIGINT will still be handled in observe_with() which waits for the observer to join and then returns to my diff in auto_restart which will stop the handler and therefore the child - but that could easily get hit by another SIGINT or some other angry signal (e.g. KILL) and break as it did before.

Having looked through this a bit more, I think my diff is somewhat incomplete anyway. By not pivoting to using the semantic exception everywhere, the behaviour is inconsistent and as an example, the observer thread is not stopped as expected since we no longer raise the expected KeyboardInterrupt. I think I can improve the behaviour as well as fixing #543 by updating the diff to do this. I'll work on it now and get a diff up shortly/follow up in an edit.

@maybe-sybr
Copy link
Contributor Author

@BoboTiG - the new diff fixes #543 from what I can tell. I've only sanity tested it manually on Linux but IIUC since we're limitiing ourselves to catching TERM and INT, it should work fine on Windows. I've also made a small doco change to discourage just catching KeyboardInterrupt in downstream code - lmk if you'd like me to remove that from the changeset prior to merge.

@BoboTiG
Copy link
Collaborator

BoboTiG commented Oct 21, 2020

Thanks a lot, that's perfect!

May I ask you to update the changelog + add youself to the line about our beloved contributors?

This fixes misbehaviour where the child process could be orphaned if
watchmedo received multiple non-fatal signals causing the
`handler.stop()` call to be interrupted or never occur. By raising a
semantic exception after neutering the relevant signals, we give
ourselves a better chance of killing the child.

Fixes gorakhargosh#543
This should encourage better handling of non-^C exit conditions in API
based use.
@maybe-sybr
Copy link
Contributor Author

Done, @BoboTiG - lmk if you need anything else from me. And thanks for being so responsive!

@BoboTiG BoboTiG merged commit 7ddd5d4 into gorakhargosh:master Oct 22, 2020
@BoboTiG
Copy link
Collaborator

BoboTiG commented Oct 22, 2020

Thank you for yours :)

@maybe-sybr maybe-sybr deleted the fix/improve-sigterm-handling branch October 22, 2020 03:05
@maybe-sybr maybe-sybr restored the fix/improve-sigterm-handling branch October 23, 2020 01:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants