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

Add a null stream IO target #24

Merged
merged 2 commits into from Jun 25, 2019
Merged

Add a null stream IO target #24

merged 2 commits into from Jun 25, 2019

Conversation

rjmac
Copy link

@rjmac rjmac commented Jun 11, 2019

This was a little more complex than I'd expected it to be. The various stream specs are all pure, and I wanted nullStream to be also, but of course it has to open the device, and the existing stream spec wasn't expressive enough to allow that. So StreamSpec has a new case specifically for a null stream, and startProcess knows how to turn that into the appropriate file handling, or fall back to a pipe-based version if that is not possible (e.g., if /dev isn't present in the current filesystem namespace).

I have not tried the Windows version, which opens \\.\NUL. I'm not sure if that'll work or not. According to Microsoft's documentation it depends which file APIs GHC uses. It is possible that just NUL is correct, but I do not have Windows to find out. update: Appveyor 🎉

Fixes #23

@rjmac
Copy link
Author

rjmac commented Jun 11, 2019

The test failure is because of cabal update failing on one GHC version. All the ones where the internet cooperated passed.

@rjmac
Copy link
Author

rjmac commented Jun 11, 2019

One last thing before I say this is ready: I'm not 100% sure the fallback-to-pipe codepath is the right thing to do as it has very slightly different semantcs. In particular, if a subprocess is launched with a handle connected to the null device and the parent process exits, output handles in the subprocess will not change. But if the parent process exits when the fallback is being used, writes to the output handles will start failing as the other end is closed.

@snoyberg
Copy link
Member

I'm not a fan of the special casing of the null case. Unless I'm misreading this, the goals can be achieved without changing the core types by separating out a nullStreamOutput that streams to /dev/null (or equivalent), or nullStreamInput for opening a pipe and then closing the write end. And I think I prefer that kind of an approach, since as it stands nullStream is doing completely different things for input and output.

Have I read the code correctly?

@rjmac
Copy link
Author

rjmac commented Jun 13, 2019

There was a version where the fallback did different things for input and output, but the final version just opens the null device for both. I wasn't a super big fan of splitting out the null stream into its own case either, but I didn't see a way to do it otherwise without making the nullStream function impure, since ultimately the device has to be opened.

One thing I did consider was dropping it here and making the change on the process package instead. That way it'd just be another standard stream type and would require no special handling here at all.

..and now that I look at it, I see you're one of the maintainers there too. If that sounds like a better plan to you, I can switch over to a PR on that which does most of the work, and change this one to just use it.

@snoyberg
Copy link
Member

What about modifying ssStream to be IO StdStream? Would that fix the entire issue?

I'm OK with entertaining a PR against process, but it wouldn't preclude the work here. typed-process tries hard to keep compat with older versions of process. I'm also very hesitant to introduce breaking changes to process, and adding an exposed constructor would be such a breakage.

@rjmac
Copy link
Author

rjmac commented Jun 13, 2019

Oh, yes it would, I think! I'll walk down that path tonight. Thanks for the hand-holding here, this is very helpful.

@snoyberg
Copy link
Member

snoyberg commented Jun 13, 2019 via email

@rjmac
Copy link
Author

rjmac commented Jun 24, 2019

Sorry, it just occurred to me that github doesn't notify for mere updates to PRs, only comments. So, I've switched the a more general format for ssStream in the latest version. It isn't quite just IO StdStream because I want to close the null device promptly (once the child has started there's no point holding on to the open device in the parent) but it's close.

src/System/Process/Typed.hs Outdated Show resolved Hide resolved
src/System/Process/Typed.hs Show resolved Hide resolved
@snoyberg snoyberg merged commit 35809cb into fpco:master Jun 25, 2019
@snoyberg
Copy link
Member

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.

closed is almost never what you want, at least on Unixlikes
2 participants