Handle SIGTERM properly, on unix systems#7921
Conversation
8bd6208 to
da1fa13
Compare
|
cabal run and cabal exec should use |
|
I agree that it would make sense for |
d0c6338 to
09e7c77
Compare
|
@robx I could give a try to this on windows, what build step could be more appropriate? |
Thanks, that'd be great! The
Just 1 would be great already. More detail there: Then on unixy systems, what I'd do is:
That's with cabal prior to this PR. After this PR, |
we can do some things in the shell in windows too :-P thanks for the tips, will follow them |
|
Hi i ve got to build this pr and test it:
In the shell running the script there is |
Sometimes it prints
|
|
Thanks a lot! I've done a bit of digging, and it seems that signals as such don't exist meaningfully on Windows / mingw. Best reference I found is https://sourceforge.net/p/mingw/mailman/message/16209215/. The
I've also checked, and the "can only be terminated forcefully" thing applies to any Haskell processes, so seems like the PR doesn't break anything, either. I think what I'll do is
Anyway, effect should be that we just get the better Ctrl-C exit on Windows, and the SIGTERM fix on Unix only. |
|
(There's probably something the ghc windows runtime could do to make it respond to ... This looks like the spot in the GHC runtime: https://gitlab.haskell.org/ghc/ghc/-/blob/master/rts/win32/ConsoleHandler.c#L214) |
Will this be addressed? |
I didn't intend to address this, at least not in this PR, since it seems orthogonal to the change as is. (ETA: I did address the comment in so far as replying to it above, did you see that?) |
|
@Mergifyio rebase master |
That's roughly how Ctrl-C (SIGINT) is handled by the GHC runtime. This way, killing cabal via SIGTERM will give it a chance to terminate any running children. Previously, it would exit directly, leaving children behind to exit on their own.
✅ Branch has been successfully rebased |
|
The cabal pre-release now includes this change and I'm getting a lot of this lately when cancelling builds: Pretty sure it's this patchset. |
|
That's on LInux. Is the problem only the alarming message or is there a visible delay or are zombies created or whatever? |
|
Quoting the PR description:
(I think process-1.6.14.0 ships with GHC 9.2.) |
|
Oh no. So I was wrong. I guess we can set a constraint in cabal.project. |
|
We could conditionally exclude the borked versions of process (if cabal is compiled on new enough GHCs that the new process can be used instead) or in cabal.project somehow, I guess. PR welcome. |
This make cabal-install reliably terminate its children before exiting when cabal-install itself is killed (fixes #7914), by:
Some notes:
withCreateProcesshelps with Ctrl-C behaviour on Windows, fixing the second half of cabal-install doesn't respect ^C #6322, as a follow-up to Fix concurrency/exception bugs in asyncFetchPackages #7929.SIGTERMchange itself doesn't affect Windows -- there appears to be no meaningful way currently to make a similar change on Windows. (A previous version of this PR used thesignalpackage to implement this in a cross-platform way, but that turns out to wrap a non-functional mingw API only.)kill -TERMofcabal runas illustrated in Cabal run leaves children running when killed #7914 works correctly with this change on unix (macos); this is verified by test.processversions before 1.6.14 (Fix waitForProcess not closing process handles with delegate_ctlc process#231) which causesspurious prints of
cabal: waitForProcess: does not exist (No child processes)when interrupting with Ctrl-C.installTerminationHandlermore widely, e.g. everywhere thattopHandleris used.Please include the following checklist in your PR:
Please also shortly describe how you tested your change. Bonus points for added tests!