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

Allow async exceptions to pierce masked waitForProcess #101

Merged
merged 3 commits into from
Aug 8, 2017

Conversation

mitchellwrosen
Copy link
Contributor

Hello!

I believe I found a bug in waitForProcess. When run in the MaskedInterruptible state, an async exception doesn't actually bring the thread down.

For example, the typed-process library exports withProcess which is similar to withCreateProcess, but ends up calling waitForProcess in the cleanup action of a bracket.

Even though waitForProcess is an interruptible foreign call, I think what's happening is:

  • Async exception enqueued for delivery to thread making waitForProcess call
  • GHC sees it's in an interruptible foreign call, sends SIGPIPE (GHC docs)
  • c_waitForProcess returns -1 with errno = EINTR
  • Upon returning to Haskell, the pending exception is not delivered, because the masking state is MaskedInterruptible
  • c_waitForProcess is retried because it's wrapped by throwErrnoIfMinus1Retry_

So our "interruptible" waitForProcess is only half-interrupted :)

My fix in this patch is to simply poll for any async exceptions before entering c_waitForProcess. Here is some related discussion I had with @snoyberg.

Thanks!

@snoyberg
Copy link
Collaborator

snoyberg commented Aug 3, 2017

This looks good, I feel much better about the usage of allowInterrupt here. For the record: I checked and the new test case does fail when allowInterrupt is not used.

@@ -81,6 +81,17 @@ main = do
unless (e1 == ExitSuccess && e2 == ExitSuccess)
$ error "sleep exited with non-zero exit code!"

do
Copy link
Collaborator

Choose a reason for hiding this comment

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

The test implementation is pretty fragile.

  • The sleep value is low enough that the process could legitimately finish before the waitForProcess begins
  • I'd rather not rely on BlockedIndefinitelyOnMVar, but instead use onException to putMVar a value in the case of an exception
  • It's worth reporting the ec in the Right case; an exit code of 0 vs 127 will be highly informative, for instance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated - how's that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good! One last request: can you add a comment to the ChangeLog explaining the change in semantics here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@snoyberg snoyberg merged commit cea82c9 into haskell:master Aug 8, 2017
@snoyberg
Copy link
Collaborator

snoyberg commented Aug 8, 2017

Thanks!

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