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

Deprecate withProcess/withProcess_ (fixes #25) #26

Merged
merged 3 commits into from Jun 26, 2019

Conversation

Projects
None yet
2 participants
@snoyberg
Copy link
Member

commented Jun 25, 2019

@psibi would you mind taking a look at this?

withProcessWait_ config f = bracket
(startProcess config)
stopProcess
(\p -> f p <* checkExitCode p)

This comment has been minimized.

Copy link
@psibi

psibi Jun 25, 2019

Member

Any reason why on this case (and in withProcessWait), you have chosen <* as compared to finally ?

This comment has been minimized.

Copy link
@snoyberg

snoyberg Jun 26, 2019

Author Member

Yes. If there was an exception in the inner block, we want that exception to propagate. Checking the exit code of the process is irrelevant when we're already about to throw a runtime exception.

@psibi

psibi approved these changes Jun 25, 2019

Copy link
Member

left a comment

LGTM, just a minor question.

@psibi

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

But strangely the test case you have added are failing in CI: https://travis-ci.org/fpco/typed-process/jobs/550165307 (Although it does pass for me locally).

@snoyberg

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2019

I'm guessing that's a timing issue, I'll try extending the sleep and seeing what happens.

@snoyberg

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2019

Nope, that didn't fix it at all, something else is going on.

@psibi

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

@snoyberg I think it might have got to do something with GHC. I changed the resolver to 9.21 and I was able to reproduce the test failures consistently.

@snoyberg

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2019

You're awesome @psibi!

@snoyberg

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2019

OK, looks like the issue is that waitForProcess isn't interruptible, and therefore the process doesn't get stopped correctly. But only on older GHCs. I'm going to comment out these two test cases and link to this comment as an explanation for why. Thanks for helping me not be crazy here @psibi, much appreciated!

@snoyberg

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2019

Last test failure, funnily enough, is the Stack bug that initially encouraged the creation of this PR.

@snoyberg snoyberg merged commit cb02c59 into master Jun 26, 2019

2 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@snoyberg snoyberg deleted the 25-deprecate-with-process branch Jun 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.