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

Should with_stdin stream content in? #28

Open
epage opened this issue Jul 19, 2018 · 1 comment
Open

Should with_stdin stream content in? #28

epage opened this issue Jul 19, 2018 · 1 comment
Labels
breaking-change enhancement Improve the expected question Uncertainty is involved

Comments

@epage
Copy link
Contributor

epage commented Jul 19, 2018

For #24 we added support for passing a file's contents to stdin (see #26).

Should we make it stream contents in?

  • From a file, read and pass in a chunk at a time
  • Support a Read object being passed in
@epage epage added enhancement Improve the expected question Uncertainty is involved breaking-change labels Jul 19, 2018
@luser
Copy link

luser commented Jul 27, 2018

Aside from the API nicety of allowing a Read object (which could be nice), the only thing this buys you is lower memory usage by not storing the entire file contents. You have to finish writing stdin before you call wait or wait_with_output because those close stdin before waiting to avoid deadlock. You actually might want to consider refactoring the current code to write stdin on a separate thread and then read stdout/stderr in parallel to avoid possible deadlock. Right now if the spawned process produces enough output to fill up its stdout or stderr buffers it'll block trying to write those, and if you are still writing stdin data you'll be blocked as well. I've got a wait_with_input_output function that does this in sccache which you can copy if you'd like. (Linking to an older version because the current source uses tokio-io and futures.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change enhancement Improve the expected question Uncertainty is involved
Projects
None yet
Development

No branches or pull requests

2 participants