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

Indicate how ChildStd{in,out,err} FDs are closed. #44625

Merged
merged 4 commits into from
Sep 24, 2017

Conversation

frewsxcv
Copy link
Member

Fixes #41452.

///
/// This struct is used in the [`stdin`] field on [`Child`].
///
/// When an instance of `ChildStdin` is [dropped], the `ChildStdin`'s underlying
/// operating system file descriptor will be closed.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'underlying operating system file descriptor' is a mouthful. very open to other terminology here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"file handle", maybe? that's just trading one OS's terminology for another's

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm 🆒 with 'file handle'. do you think "underlying operating system file handle" or "underlying file handle" or "file handle" or something else?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"underlying file handle" was what i had in mind

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the idea! 'underlying file handle' incorporated in the latest force push

@frewsxcv
Copy link
Member Author

r? @QuietMisdreavus

@frewsxcv
Copy link
Member Author

i thought about throwing 'RAII" somewhere in here but could think of a good way to incorporate it

@abonander
Copy link
Contributor

Maybe mention for ChildStdin that, if the child process is blocked on input, it will become unblocked? (unless some OS blocks forever when a pipe is closed)

@QuietMisdreavus
Copy link
Member

@abonander maybe something about how it'll put an end-of-file at the end of whatever's already been written to it?

@@ -534,7 +549,8 @@ impl Command {
self
}

/// Configuration for the child process's stdin handle (file descriptor 0).
/// Configuration for the child process's standard input (stdin) handle
/// (file descriptor 0).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file descriptor 0 part seems awfully platform specific.

@carols10cents carols10cents added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 18, 2017
@frewsxcv
Copy link
Member Author

@abonander I'm not too familiar with the behavior/semantics of pipes w.r.t this. Does this sound right?

When an instance of ChildStdin is [dropped], the ChildStdin's underlying file handle will be closed. If the child process was blocked on input prior to being dropped, it will become unblocked after dropping.

@frewsxcv
Copy link
Member Author

Added a couple more commits. Anyone else have thoughts?

@QuietMisdreavus
Copy link
Member

Seems good to me. @abonander What do you think about the note of child processes being unblocked?

@abonander
Copy link
Contributor

abonander commented Sep 22, 2017

I can't say whether or not it's correct for all platforms but it answers OP's question in the Reddit post that spawned #41452 so I'm happy with it.

@QuietMisdreavus
Copy link
Member

Since this seems to be all set, i'll go ahead and get this in.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 23, 2017

📌 Commit 859ebef has been approved by QuietMisdreavus

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 23, 2017
…etMisdreavus

Indicate how ChildStd{in,out,err} FDs are closed.

Fixes rust-lang#41452.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 24, 2017
…etMisdreavus

Indicate how ChildStd{in,out,err} FDs are closed.

Fixes rust-lang#41452.
bors added a commit that referenced this pull request Sep 24, 2017
Rollup of 4 pull requests

- Successful merges: #44103, #44625, #44789, #44795
- Failed merges:
@bors bors merged commit 859ebef into rust-lang:master Sep 24, 2017
mizzy added a commit to libspecinfra/specinfra that referenced this pull request Nov 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants