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.

Sign up for GitHub

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

Jump to bottom

std: Child::kill() returns error if process has already exited #49461

Merged
merged 5 commits into from
Apr 24, 2018
Merged

std: Child::kill() returns error if process has already exited #49461

merged 5 commits into from
Apr 24, 2018

Conversation

andreastt
Copy link
Contributor

This patch makes it clear in std::process::Child::kill()'s API
documentation that an error is returned if the child process has
already cleanly exited. This is implied by the example, but not
called out explicitly.

@andreastt
std: Child::kill() returns error if process has already exited
7eb9a09 
This patch makes it clear in std::process::Child::kill()'s API
documentation that an error is returned if the child process has
already cleanly exited.  This is implied by the example, but not
called out explicitly.
Mark-Simulacrum
Mark-Simulacrum reviewed Mar 28, 2018
View reviewed changes
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

Could we note which error is returned, at least on common platforms, though we should indicate it's not guaranteed to be the case.

@andreastt
Copy link
Contributor Author

andreastt commented Mar 28, 2018 •
edited
Loading

Certainly we could.

Helpfully, @whimboo provided an example of such an error in https://bugzilla.mozilla.org/show_bug.cgi?id=1448900#c3. I’ve referenced InvalidInput but I’m not sure how useful it is.

When you say it is not guaranteed that it returns an error if the process has already exited, which systems are you referring to?

@Mark-Simulacrum
Copy link
Member

I meant that the error returned today on a given system might change down the road and/or other situations could return new errors; we can't guarantee stability of underlying OS APIs.

@andreastt
Copy link
Contributor Author

And do you think the amendment resolves this issue? Specifically it s/is returned/might be returned/.

@Mark-Simulacrum
Copy link
Member

I'd rather leave that as is returned and then have a note in a separate paragraph, somewhat like what we do here: https://doc.rust-lang.org/nightly/std/fs/struct.OpenOptions.html#errors (with the "The mapping to ErrorKinds is not part of the compatibility contract of the function, especially the Other kind might change to more specific kinds in the future." part).

@andreastt
Copy link
Contributor Author

Thanks for your feedback. I’ve pushed another fixup which takes that into consideration. Let me know what you think.

@shepmaster
Copy link
Member

r? @Mark-Simulacrum

@shepmaster shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 30, 2018
Mark-Simulacrum
Mark-Simulacrum reviewed Mar 30, 2018
View reviewed changes
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

r=me with the nit fixed

src/libstd/process.rs Outdated
///
/// [`ErrorKind`]: ../io/enum.ErrorKind.html
/// [`InvalidInput`]: ../io/enum.ErrorKind.html#variant.InvalidInput
/// [`Other]: ../io/enum.ErrorKind.html#variant.Other
Copy link
Member

Choose a reason for hiding this comment

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

Missing a '`' on Other

@shepmaster shepmaster added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 7, 2018
@shepmaster
Copy link
Member

Ping from triage @andreastt — you have a review comment to address!

@andreastt
Copy link
Contributor Author

Sorry about that; nit addressed.

@pietroalbini pietroalbini added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 16, 2018
@pietroalbini
Copy link
Member

Ping from triage @Mark-Simulacrum! The user addressed your comments.

Mark-Simulacrum
Mark-Simulacrum reviewed Apr 16, 2018
View reviewed changes
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

r=me with the nit fixed

src/libstd/process.rs Outdated
@@ -1121,8 +1121,13 @@ impl ExitCode {
}

impl Child {
/// Forces the child to exit. This is equivalent to sending a
/// SIGKILL on unix platforms.
/// Forces the child process to exit. If the child has already exited, an [`InvalidInput`]
Copy link
Member

Choose a reason for hiding this comment

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

nit: Double space after the period.

@pietroalbini
Copy link
Member

Ping from triage @Mark-Simulacrum! The user addressed your comments.

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 23, 2018

📌 Commit bc4bd56 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 23, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Apr 24, 2018
@kennytm
Rollup merge of rust-lang#49461 - andreastt:child-kill-exited, r=Mark…
cefdd6d 
…-Simulacrum

std: Child::kill() returns error if process has already exited

This patch makes it clear in std::process::Child::kill()'s API
documentation that an error is returned if the child process has
already cleanly exited.  This is implied by the example, but not
called out explicitly.
@kennytm kennytm mentioned this pull request Apr 24, 2018
Merged
bors added a commit that referenced this pull request Apr 24, 2018
@bors
Auto merge of #50191 - kennytm:rollup, r=kennytm
52ed3d8 
Rollup of 11 pull requests

Successful merges:

 - #49461 (std: Child::kill() returns error if process has already exited)
 - #49727 (Add Cell::update)
 - #49812 (Fix revision support for UI tests.)
 - #49829 (Add doc links to `std::os` extension traits)
 - #49906 (Stabilize `std::hint::unreachable_unchecked`.)
 - #49970 (Deprecate Read::chars and char::decode_utf8)
 - #49985 (don't see issue #0)
 - #50118 (fix search bar bug)
 - #50139 (encourage descriptive issue titles)
 - #50174 (Use FxHashMap in syntax_pos::symbol::Interner::intern.)
 - #50185 (core: Fix overflow in `int::mod_euc` when `self < 0 && rhs == MIN`)

Failed merges:
@bors bors merged commit bc4bd56 into rust-lang:master Apr 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@andreastt @Mark-Simulacrum @shepmaster @pietroalbini @bors