Skip to content

Terminate child processes on sigINT/sigTERM #7757

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

Conversation

hasufell
Copy link
Member

@hasufell hasufell commented Oct 16, 2021

For more information, check out ndmitchell/shake#169

Questions that may need to be discussed:

  1. Is the additional dependency allowed? It's a small single-module package that could be incorporated. It seems to work even on windows, utilizing some foreign calls (also see MS documentation), maybe @Mistuke can comment
  2. I don't see an obvious way to clean up the handlers themselves cross-platform. Will consecutive calls to running external binaries cause problems here?
  3. How safe is this in general?
  4. Another approach is to use exec (at least for cabal run/exec), but that doesn't seem to be available on windows.

@hasufell hasufell force-pushed the hasufell/PR/terminate-child-processes branch from 7b6f469 to 5b1d79a Compare October 16, 2021 12:36
@hasufell hasufell changed the title Terminate child processes on sigINT/sigTERM etc Terminate child processes on sigINT/sigTERM Oct 16, 2021
@hasufell
Copy link
Member Author

Note that we don't really re-throw the signal to the child, but rather run cleanupProcess, which runs terminateProcess, which runs:

@hasufell
Copy link
Member Author

So I tracked down the callstack for the downloaders that invoke curl etc... and it seems they also invoke rawSystemStdInOut asynchronously via withAsync. I assume we won't really kill all curl processes that way?

@hasufell
Copy link
Member Author

Another possibility is to do it like rio:

https://hackage.haskell.org/package/rio-0.1.21.0/docs/src/RIO.Process.html#exec

-- | Execute a process within the configured environment.
--
-- Execution will not return, because either:
--
-- 1) On non-windows, execution is taken over by execv of the
-- sub-process. This allows signals to be propagated (#527)
--
-- 2) On windows, an 'ExitCode' exception will be thrown.
--
-- @since 0.0.3.0
exec :: (HasProcessContext env, HasLogFunc env) => String -> [String] -> RIO env b
#ifdef WINDOWS
exec = execSpawn
#else
exec cmd0 args = do
    wd <- view workingDirL
    envStringsL <- view envVarsStringsL
    cmd <- preProcess cmd0
    withProcessTimeLog wd cmd args $ liftIO $ do
      for_ wd setCurrentDirectory
      executeFile cmd True args $ Just envStringsL
#endif

But this seems to me it would only work nicely on unix.

@robx
Copy link
Collaborator

robx commented Jan 20, 2022

Very much in favour of doing something here! I think cabal should ensure all its children exit on SIGTERM.

Some vague thoughts:

  • I agree that for run and exec, exec(3)-like behavior would be ideal. It also seems like it would be worthwhile fixing run/exec first if that's easier.
  • Regarding the lack of clean-up, I'm also a bit unsure what it means to keep all of these handlers around. I imagine we might run into issues such as running out of file descriptors keeping all those fd handles around? I think multiple signal handlers don't work -- previous signal handlers are replaced by newer ones -- which would make this sort of work accidentally.
  • I'm wondering whether we could do something like:
    • install SIGTERM handler that swallows the signal and throws an async exception at the main thread (not exactly sure whether that's sensible and can be achieved)
    • bracket the createProcess calls with cleanupProcess
      EDIT: proof of concept PR: Handle SIGTERM properly, on unix systems #7921

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.

2 participants