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

Unify execute_command across platforms #193

Merged
merged 1 commit into from
Dec 10, 2017
Merged

Unify execute_command across platforms #193

merged 1 commit into from
Dec 10, 2017

Conversation

reima
Copy link
Contributor

@reima reima commented Dec 9, 2017

Fixes #178 by providing an empty stdin, which leads to the command not blocking on input.

To achieve this, I removed the #[cfg(all(unix))] implementation of execute_command in favor of the simpler #[cfg(not(unix))] implementation (which is now used unconditionally). This implementation uses Command::output, which by default captures stdout and stderr and provides an empty stdin.

Incidentally, this also fixes an (unreported) bug in the removed execute_command implementation: when the command output grew larger than the size of a pipe, a deadlock occured:

  • The command was blocked on writing to the filled pipe.
  • fd was blocked on waiting for the command to finish.

I'm not entirely sure why we even had the removed execute_command to begin with, as using the Command's built-in output capture features seem to work just fine on Unix configurations.

@jakwings
Copy link
Contributor

jakwings commented Dec 9, 2017

@mmstick /cc

I also tried using Command::output, but it doen't capture anything on my mac. So, that separate implementation is a fix for unixes, not RedoxOS.

@jakwings
Copy link
Contributor

jakwings commented Dec 9, 2017

Ah, sorry, I said it wrong. It does capture output but there seemed to be something wrong with stdin. I don't remember.

@mmstick
Copy link
Contributor

mmstick commented Dec 9, 2017

Is STDIN_FILENO being closed in the child before executing the command?

@jakwings
Copy link
Contributor

jakwings commented Dec 9, 2017

Yes, the stdin is closed. (checked with libc::isatty in .before_exec())

But the doc said Command::output: By default, stdin, stdout and stderr are captured (and used to provide the resulting output).

@reima
Copy link
Contributor Author

reima commented Dec 9, 2017

@mmstick Yes. Technically std::process::Command::output excutes the command first with the command's stdin set to /dev/null (null: on Redox, NUL on Windows), and then closes the command's stdin in std::process::Child::wait_with_output.

@sharkdp sharkdp merged commit 5784823 into sharkdp:master Dec 10, 2017
@sharkdp
Copy link
Owner

sharkdp commented Dec 10, 2017

Thank you very much!

@jakwings
Copy link
Contributor

This issue confirm the behavior of Command::output: rust-lang/rust#46220

@reima reima deleted the feature/exec-input-fix branch December 15, 2017 14:30
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.

hang with exec
4 participants