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

withCreateProcess async exception safety #183

Open
joeyh opened this issue Jun 9, 2020 · 2 comments
Open

withCreateProcess async exception safety #183

joeyh opened this issue Jun 9, 2020 · 2 comments

Comments

@joeyh
Copy link
Contributor

joeyh commented Jun 9, 2020

I'm concerned that withCreateProcess may fail to clean up if an async exception is received at just the wrong time. I do not have a test case to prove it, but my reasoning follows.

See Control.Exception's discussion of "Interruptible operations". The masking of async exceptions by bracket when cleanupProcess is running only prevents non-interruptable operations from receiving an async exception. So if any operation in cleanupProcess turns out to be interruptable, an async exception received during that operation will prevent the rest of it from running.

Looking at the operations that get run, there's a takeMVar, but I think the MVar is always full, so it's not interruptable. Then it closes the IO handles, and as far as I can see, hClose is interruptable, or at least is not documented not to be.

So, an async exception received just as hClose is run will terminate the thread, and waitForProcess will never wait on the process. (IO handles could also potentially be left open, although generally GC will auto-close them when they go out of scope.) It's probably a rather narrow race condition.

My analysis could be wrong, but eh, if I've been staring at this code for an hour and I don't know if it it's correct, it seems like it would be a good idea to make sure this doesn't happen by using uninterruptibleMask somewhere?

(The closest I have come to a test case is modifying cleanupProcess to add threadDelay 100, and then it's easy to time the async exception to interrupt it, leaving the process running. But of course it might be threadDelay itself that's the interruptable operation in that case.)

@joeyh
Copy link
Contributor Author

joeyh commented Jun 10, 2020

hClose is indeed interruptable, see the comment here
https://hackage.haskell.org/package/base-4.14.0.0/docs/src/GHC.IO.Handle.Internals.html#hClose_help

An alternative to uninterruptibleMask would be to work off a worker thread to do all the closing etc, and wait on it before returning. That insulates it from receiving async exceptions, and won't prevent the program from being interrupted by ctrl-c while it's running.

@snoyberg
Copy link
Collaborator

snoyberg commented Jun 11, 2020

This topic is an ongoing debate in the Haskell community. I agree with uninterruptible being the right behavior for cleanups, and wish bracket worked that way. I'm not sure if it's a good idea to change an existing function in a core library with a difficult-to-notice semantics change, however.

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

No branches or pull requests

3 participants
@joeyh @snoyberg and others