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

WIP: make waitForProcess async safe #204

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

snoyberg
Copy link
Collaborator

Moving from fpco/typed-process#38. CC @NorfairKing

This PR is not complete, the tests fail. I'm trying to determine whether this is a reasonable approach. My understanding of the issue here:

  1. waitForProcess wants to allow the C FFI call c_waitForProcess to be interruptible, so that waitpid can be interrupted
  2. If an async exception lands at exactly the wrong time, the waitpid call with succeed, but the FFI call will throw an exception due to the async exception
  3. In this case, the system process table no longer has an entry for the given PID, but the MVar inside ProcessHandle believes that the handle is still open, allowing future waitForProcess calls to occur
  4. Future calls throw an exception, since it's using an unknown PID

My best guess is that, in order to both allow waitpid to be interrupted and to reliably capture the exit code if it's generated, we need to:

  1. No longer rely on return codes from c_waitForProcess, since those will be lost in the case of an async exception
  2. Therefore, we need to detect whether the waitpid call succeeded or not by passing in another int* (along with the existing output parameter pret).
  3. Even if c_waitForProcess throws an exception, we need to update the MVar with the exit code, if it's returned successfully

I've taken a stab at this in this PR. @simonmar I was hoping you could have a look and let me know if this approach works, and/or if there's a better way to attack this kind of problem.

@NorfairKing
Copy link

@snoyberg Thanks for CC-ing me. I don't feel qualified to review this PR, to be honest. I'm betting that @nh2 will, if he has some spare cycles to help with this.

@nh2
Copy link
Member

nh2 commented Mar 23, 2021

I thought a bit about it and will try to reply to reply tomorrow.

@NorfairKing
Copy link

@simonmar Gentle ping about this.

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.

3 participants