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

Tracking issue for Child::try_wait #38903

Closed
alexcrichton opened this issue Jan 7, 2017 · 19 comments
Closed

Tracking issue for Child::try_wait #38903

alexcrichton opened this issue Jan 7, 2017 · 19 comments
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

alexcrichton commented Jan 7, 2017

Introduced in #38866 this is a tracking issue for the Child::try_wait method.

This method attempts to wait for a child and collect its status but does not block. Questions for stabilization include:

  • Is the method name suitable?
  • Is the return type suitable? (io::Result<T> vs io::Result<Option<T>>)
@alexcrichton alexcrichton added B-unstable Blocker: Implemented in the nightly compiler and unstable. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 7, 2017
@SimonSapin
Copy link
Contributor

If this does not block, there’s not waiting involved?

@oconnor663
Copy link
Contributor

oconnor663 commented Jan 24, 2017

One problem I'm grappling with right now is how to wait on a child (in one thread) while leaving open the option of killing it later (from another thread) if it hasn't exited yet. Right now the wait method takes &mut self, so there's no way to call kill (which also takes &mut self) if someone's waiting on wait, and it would be a race condition anyway. The Unix way seems to be listening to SIGCHILD, but that assumes I own the whole process's signal handlers, and I don't want to assume that in library code.

try_wait will help. I can call that in a loop with sleeps in-between. That's doesn't feel great, but it's what Python does. A perfect solution might be to call the newer waitid function with WNOWAIT, which waits without reaping the child, to avoid racing against kill. But that one might not be defined on all platforms? I'm not sure yet. It does seem like it'll be easier to use waitid out of tree where available, since it's still safe (actually necessary to avoid leaks?) to call the child's wait afterwards.

If it's true that waitid "subsumes the rest of the wait* family" (according to that link above), it might be nice to expose it at some point? I wouldn't want to derail this issue with that question, but I'm interested in thinking about what it would look like. If a hypothetical future wait_without_waiting method/koan took &self, it would still be impossible to call kill while another thread was waiting. What API would make that possible? Maybe returning some sort of KillHandle that borrows the Child? I think it's unspecified what happens if multiple threads are waiting at once on the same child, so preventing that might be a good thing.

@alexcrichton
Copy link
Member Author

@oconnor663 what you're trying to do is inherently race-prone on Unix and &mut is simply expressing that. A pid is consumed in the ownership sense once wait succeeds, so a call to kill could be a use-after-free if it's not synchronized with wait.

It does indeed sound like waitid would be quite nice! If it's implemented across platforms then I think we could definitely start using it and consider relaxing &mut self and such.

@oconnor663
Copy link
Contributor

oconnor663 commented Jan 24, 2017

Golang added (private) support for waitid six months ago, and it looks like they only did it on Linux. Ah well.

@oconnor663
Copy link
Contributor

Hmm, rust-lang/libc#489 seems to have included Mac support though. I'll play with it. (Interesting timing!)

@oconnor663
Copy link
Contributor

oconnor663 commented Feb 1, 2017

Thinking more about how I'm going to expose a similar API in duct. I think I'm going to opt for io::Result<Option<ExitStatus>>. My reasoning is that most callers are going to want to short-circuit on "real errors", and I want them to look like this

if let Some(status) = foo.try_wait()? {
    ...
} else {
    ...
}

instead of like this

match foo.try_wait() {
    Ok(status) => {
        ...
    }
    Err(err) if err.kind() == io::ErrorKind::WouldBlock => {
        ...
    }
    Err(err) => return Err(err),
}

The non-blocking Read and Write APIs don't work this way, because they have to support both blocking and non-blocking callers in the same function signature, and it's nasty to force the common blocking case to unwrap an Option. But the try_wait API here is never blocking, and probably none of its callers are going to want to treat the WouldBlock condition as a real error.

Name-wise, I agree with @SimonSapin that the term "try_wait" is kind of confusing, though I guess it's consistent with what libc calls everything, and with the Mutex API. (The gold medal winner for confusing names in this part of the standard has got to be waitid(...WNOWAIT).) I could see check or poll? But those don't really convey the "and also clean up the child if it's finished" part of the function, which is pretty important.

@alexcrichton
Copy link
Member Author

@oconnor663 good point about using Option instead of just io::Result!

Also yeah the name is mostly inspired by mutexes and other nonblocking/opportunistic operations which are prefixed with try_ (e.g. try_recv, try_borrow, etc)

@oconnor663
Copy link
Contributor

@alexcrichton would you be interested in a PR that tried out the Option approach?

@alexcrichton
Copy link
Member Author

Oh oops I thought that it already did that actually. Sure if you want to send a PR sounds good to me!

@oconnor663
Copy link
Contributor

oconnor663 commented Feb 7, 2017

We merged are in the middle of merging #39512, but @nagisa mentioned in there that they were worried about adding more inconsistency to our async APIs, and I worry about that too. Here's some of the existing work that we'll want to think about:

  • std::io::Read and friends, which use std::io::Result with ErrorKind::WouldBlock
  • try_lock, which uses a custom TryLockError enum with a WouldBlock case of its own
  • the futures crate, which defines the Poll type to be Result<Async<T>, E>, very similar to Result<Option<T>, E>

oconnor663 added a commit to oconnor663/rust that referenced this issue Feb 7, 2017
This is much nicer for callers who want to short-circuit real I/O errors
with `?`, because they can write this

    if let Some(status) = foo.try_wait()? {
        ...
    } else {
        ...
    }

instead of this

    match foo.try_wait() {
        Ok(status) => {
            ...
        }
        Err(err) if err.kind() == io::ErrorKind::WouldBlock => {
            ...
        }
        Err(err) => return Err(err),
    }

The original design of `try_wait` was patterned after the `Read` and
`Write` traits, which support both blocking and non-blocking
implementations in a single API. But since `try_wait` is never blocking,
it makes sense to optimize for the non-blocking case.

Tracking issue: rust-lang#38903
frewsxcv added a commit to frewsxcv/rust that referenced this issue Feb 8, 2017
make Child::try_wait return io::Result<Option<ExitStatus>>

This is much nicer for callers who want to short-circuit real I/O errors
with `?`, because they can write this

    if let Some(status) = foo.try_wait()? {
        ...
    } else {
        ...
    }

instead of this

    match foo.try_wait() {
        Ok(status) => {
            ...
        }
        Err(err) if err.kind() == io::ErrorKind::WouldBlock => {
            ...
        }
        Err(err) => return Err(err),
    }

The original design of `try_wait` was patterned after the `Read` and
`Write` traits, which support both blocking and non-blocking
implementations in a single API. But since `try_wait` is never blocking,
it makes sense to optimize for the non-blocking case.

Tracking issue: rust-lang#38903
frewsxcv added a commit to frewsxcv/rust that referenced this issue Feb 8, 2017
make Child::try_wait return io::Result<Option<ExitStatus>>

This is much nicer for callers who want to short-circuit real I/O errors
with `?`, because they can write this

    if let Some(status) = foo.try_wait()? {
        ...
    } else {
        ...
    }

instead of this

    match foo.try_wait() {
        Ok(status) => {
            ...
        }
        Err(err) if err.kind() == io::ErrorKind::WouldBlock => {
            ...
        }
        Err(err) => return Err(err),
    }

The original design of `try_wait` was patterned after the `Read` and
`Write` traits, which support both blocking and non-blocking
implementations in a single API. But since `try_wait` is never blocking,
it makes sense to optimize for the non-blocking case.

Tracking issue: rust-lang#38903
frewsxcv added a commit to frewsxcv/rust that referenced this issue Feb 8, 2017
make Child::try_wait return io::Result<Option<ExitStatus>>

This is much nicer for callers who want to short-circuit real I/O errors
with `?`, because they can write this

    if let Some(status) = foo.try_wait()? {
        ...
    } else {
        ...
    }

instead of this

    match foo.try_wait() {
        Ok(status) => {
            ...
        }
        Err(err) if err.kind() == io::ErrorKind::WouldBlock => {
            ...
        }
        Err(err) => return Err(err),
    }

The original design of `try_wait` was patterned after the `Read` and
`Write` traits, which support both blocking and non-blocking
implementations in a single API. But since `try_wait` is never blocking,
it makes sense to optimize for the non-blocking case.

Tracking issue: rust-lang#38903
@Timidger
Copy link

Any updates on this issue? Right now this function leaves a hole in the lockscreen for Way Cooler where the lock screen program could have failed but we can't clean the input locks / stored PID properly because there's no way to check the status of the program without blocking.

@oconnor663
Copy link
Contributor

oconnor663 commented Mar 26, 2017

@Timidger is there any chance you could use the shared_child crate, which exposes a try_wait function now? There might be some missing features (particularly if you're collecting stdout), but I'd be happy to add them.

@Timidger
Copy link

@oconnor663 Thanks, it looks like that crate will work. Would be nice for this to be in the stable standard library thought :-/. Until this stabilizes I'll try to use that then.

@alexcrichton
Copy link
Member Author

Aha and that reminds me!

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Mar 27, 2017

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

anatol pushed a commit to anatol/steed that referenced this issue Mar 31, 2017
This is much nicer for callers who want to short-circuit real I/O errors
with `?`, because they can write this

    if let Some(status) = foo.try_wait()? {
        ...
    } else {
        ...
    }

instead of this

    match foo.try_wait() {
        Ok(status) => {
            ...
        }
        Err(err) if err.kind() == io::ErrorKind::WouldBlock => {
            ...
        }
        Err(err) => return Err(err),
    }

The original design of `try_wait` was patterned after the `Read` and
`Write` traits, which support both blocking and non-blocking
implementations in a single API. But since `try_wait` is never blocking,
it makes sense to optimize for the non-blocking case.

Tracking issue: rust-lang/rust#38903
anatol pushed a commit to anatol/steed that referenced this issue Mar 31, 2017
make Child::try_wait return io::Result<Option<ExitStatus>>

This is much nicer for callers who want to short-circuit real I/O errors
with `?`, because they can write this

    if let Some(status) = foo.try_wait()? {
        ...
    } else {
        ...
    }

instead of this

    match foo.try_wait() {
        Ok(status) => {
            ...
        }
        Err(err) if err.kind() == io::ErrorKind::WouldBlock => {
            ...
        }
        Err(err) => return Err(err),
    }

The original design of `try_wait` was patterned after the `Read` and
`Write` traits, which support both blocking and non-blocking
implementations in a single API. But since `try_wait` is never blocking,
it makes sense to optimize for the non-blocking case.

Tracking issue: rust-lang/rust#38903
@rfcbot
Copy link

rfcbot commented Apr 11, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Apr 11, 2017
@oconnor663
Copy link
Contributor

@alexcrichton, what actually happens at the end of the FCP?

@sfackler
Copy link
Member

@oconnor663 the method is stabilized and will land in the next release or the one after that depending on the timing.

@rfcbot
Copy link

rfcbot commented Apr 21, 2017

The final comment period is now complete.

sfackler added a commit to sfackler/rust that referenced this issue May 21, 2017
bors added a commit that referenced this issue May 21, 2017
Stabilize library features for 1.18.0

Closes #38863
Closes #38980
Closes #38903
Closes #36648

r? @alexcrichton

@rust-lang/libs
brson pushed a commit to brson/rust that referenced this issue May 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants