Tracking issue for CommandExt::before_exec #31398

alexcrichton · 2016-02-04T00:22:36Z

Open questions:

The requirements for Send + Sync on the closure need to be documented and are "somewhat odd"
It's somewhat surprising to have to deal with a list of callbacks to invoke, especially when order might matter quite a bit.
There are weird semantics with using Stdio::piped() for any of the stdio handles because they'll immediately close the other half of the pipe (via CLOEXEC).

This is a tracking issue for rust-lang/rfcs#1359.

The text was updated successfully, but these errors were encountered:

dirk · 2016-02-04T18:15:12Z

I'm planning on giving this a shot this evening. 😄

alexcrichton · 2016-02-04T18:25:28Z

@dirk oh sorry I should have mentioned that I'm working on implementing this. There's actually a good deal of refactoring I'd like to do at the same time, and I'm already halfway through it so far!

dirk · 2016-02-04T18:28:12Z

@alexcrichton: Oh, no worries! Thanks for replying so quickly.

This is a Unix-specific function which adds the ability to register a closure to run pre-exec to configure the child process as required (note that these closures are run post-fork). cc rust-lang#31398

This commit implements the `exec` function proposed in [RFC 1359][rfc] which is a function on the `CommandExt` trait to execute all parts of a `Command::spawn` without the `fork` on Unix. More details on the function itself can be found in the comments in the commit. [rfc]: rust-lang/rfcs#1359 cc rust-lang#31398

These commits are an implementation of rust-lang/rfcs#1359 which is tracked via #31398. The `before_exec` implementation fit easily with the current process spawning framework we have, but unfortunately the `exec` implementation required a bit of a larger refactoring. The stdio handles were all largely managed as implementation details of `std::process` and the `exec` function lived in `std::sys`, so the two didn't have access to one another. I took this as a sign that a deeper refactoring was necessary, and I personally feel that the end result is cleaner for both Windows and Unix. The commits should be separated nicely for reviewing (or all at once if you're feeling ambitious), but the changes made here were: * The process spawning on Unix was refactored in to a pre-exec and post-exec function. The post-exec function isn't allowed to do any allocations of any form, and management of transmitting errors back to the parent is managed by the pre-exec function (as it's the one that actually forks). * Some management of the exit status was pushed into platform-specific modules. On Unix we must cache the return value of `wait` as the pid is consumed after we wait on it, but on Windows we can just keep querying the system because the handle stays valid. * The `Stdio::None` variant was renamed to `Stdio::Null` to better reflect what it's doing. * The global lock on `CreateProcess` is now correctly positioned to avoid unintended inheritance of pipe handles that other threads are sending to their child processes. After a more careful reading of the article referenced the race is not in `CreateProcess` itself, but rather the property that handles are unintentionally shared. * All stdio management now happens in platform-specific modules. This provides a cleaner implementation/interpretation for `FromFraw{Fd,Handle}` for each platform as well as a cleaner transition from a configuration to what-to-do once we actually need to do the spawn. With these refactorings in place, implementing `before_exec` and `exec` ended up both being pretty trivial! (each in their own commit)

pnkfelix · 2016-03-09T17:11:33Z

@alexcrichton I am seeing test failures in check-stage1-rpass for src/test/run-pass/command-before-exec.rs on Mac OS X. (This is with a MIR analysis branch, with no changes to code gen; I haven't double-checked against a clean master yet, but it seems very unlikely that my own changes are related.)

Is this functionality only working for stage2 and higher?

Or is there potentially some latent bug here that I should double check?

alexcrichton · 2016-03-09T17:18:22Z

Hm in theory the test should work for all stages, so seems suspicious that it's failing! Do you have some logs that I could look at? May be able to help diagnose what's happening in any case.

pnkfelix · 2016-03-10T08:15:06Z

@alexcrichton here is a gist of a run I did locally: https://gist.github.com/pnkfelix/aa878b372761f957a9d4

(update: added actual gist link url)

Running the test binary itself on its own by hand does not reproduce the issue, and the nature of the test makes it difficult to add println! style instrumentation to the test. :)

This commit applies all stabilizations, renamings, and deprecations that the library team has decided on for the upcoming 1.9 release. All tracking issues have gone through a cycle-long "final comment period" and the specific APIs stabilized/deprecated are: Stable * `std::panic` * `std::panic::catch_unwind` (renamed from `recover`) * `std::panic::resume_unwind` (renamed from `propagate`) * `std::panic::AssertUnwindSafe` (renamed from `AssertRecoverSafe`) * `std::panic::UnwindSafe` (renamed from `RecoverSafe`) * `str::is_char_boundary` * `<*const T>::as_ref` * `<*mut T>::as_ref` * `<*mut T>::as_mut` * `AsciiExt::make_ascii_uppercase` * `AsciiExt::make_ascii_lowercase` * `char::decode_utf16` * `char::DecodeUtf16` * `char::DecodeUtf16Error` * `char::DecodeUtf16Error::unpaired_surrogate` * `BTreeSet::take` * `BTreeSet::replace` * `BTreeSet::get` * `HashSet::take` * `HashSet::replace` * `HashSet::get` * `OsString::with_capacity` * `OsString::clear` * `OsString::capacity` * `OsString::reserve` * `OsString::reserve_exact` * `OsStr::is_empty` * `OsStr::len` * `std::os::unix::thread` * `RawPthread` * `JoinHandleExt` * `JoinHandleExt::as_pthread_t` * `JoinHandleExt::into_pthread_t` * `HashSet::hasher` * `HashMap::hasher` * `CommandExt::exec` * `File::try_clone` * `SocketAddr::set_ip` * `SocketAddr::set_port` * `SocketAddrV4::set_ip` * `SocketAddrV4::set_port` * `SocketAddrV6::set_ip` * `SocketAddrV6::set_port` * `SocketAddrV6::set_flowinfo` * `SocketAddrV6::set_scope_id` * `<[T]>::copy_from_slice` * `ptr::read_volatile` * `ptr::write_volatile` * The `#[deprecated]` attribute * `OpenOptions::create_new` Deprecated * `std::raw::Slice` - use raw parts of `slice` module instead * `std::raw::Repr` - use raw parts of `slice` module instead * `str::char_range_at` - use slicing plus `chars()` plus `len_utf8` * `str::char_range_at_reverse` - use slicing plus `chars().rev()` plus `len_utf8` * `str::char_at` - use slicing plus `chars()` * `str::char_at_reverse` - use slicing plus `chars().rev()` * `str::slice_shift_char` - use `chars()` plus `Chars::as_str` * `CommandExt::session_leader` - use `before_exec` instead. Closes rust-lang#27719 cc rust-lang#27751 (deprecating the `Slice` bits) Closes rust-lang#27754 Closes rust-lang#27780 Closes rust-lang#27809 Closes rust-lang#27811 Closes rust-lang#27830 Closes rust-lang#28050 Closes rust-lang#29453 Closes rust-lang#29791 Closes rust-lang#29935 Closes rust-lang#30014 Closes rust-lang#30752 Closes rust-lang#31262 cc rust-lang#31398 (still need to deal with `before_exec`) Closes rust-lang#31405 Closes rust-lang#31572 Closes rust-lang#31755 Closes rust-lang#31756

std: Stabilize APIs for the 1.9 release This commit applies all stabilizations, renamings, and deprecations that the library team has decided on for the upcoming 1.9 release. All tracking issues have gone through a cycle-long "final comment period" and the specific APIs stabilized/deprecated are: Stable * `std::panic` * `std::panic::catch_unwind` (renamed from `recover`) * `std::panic::resume_unwind` (renamed from `propagate`) * `std::panic::AssertUnwindSafe` (renamed from `AssertRecoverSafe`) * `std::panic::UnwindSafe` (renamed from `RecoverSafe`) * `str::is_char_boundary` * `<*const T>::as_ref` * `<*mut T>::as_ref` * `<*mut T>::as_mut` * `AsciiExt::make_ascii_uppercase` * `AsciiExt::make_ascii_lowercase` * `char::decode_utf16` * `char::DecodeUtf16` * `char::DecodeUtf16Error` * `char::DecodeUtf16Error::unpaired_surrogate` * `BTreeSet::take` * `BTreeSet::replace` * `BTreeSet::get` * `HashSet::take` * `HashSet::replace` * `HashSet::get` * `OsString::with_capacity` * `OsString::clear` * `OsString::capacity` * `OsString::reserve` * `OsString::reserve_exact` * `OsStr::is_empty` * `OsStr::len` * `std::os::unix::thread` * `RawPthread` * `JoinHandleExt` * `JoinHandleExt::as_pthread_t` * `JoinHandleExt::into_pthread_t` * `HashSet::hasher` * `HashMap::hasher` * `CommandExt::exec` * `File::try_clone` * `SocketAddr::set_ip` * `SocketAddr::set_port` * `SocketAddrV4::set_ip` * `SocketAddrV4::set_port` * `SocketAddrV6::set_ip` * `SocketAddrV6::set_port` * `SocketAddrV6::set_flowinfo` * `SocketAddrV6::set_scope_id` * `<[T]>::copy_from_slice` * `ptr::read_volatile` * `ptr::write_volatile` * The `#[deprecated]` attribute * `OpenOptions::create_new` Deprecated * `std::raw::Slice` - use raw parts of `slice` module instead * `std::raw::Repr` - use raw parts of `slice` module instead * `str::char_range_at` - use slicing plus `chars()` plus `len_utf8` * `str::char_range_at_reverse` - use slicing plus `chars().rev()` plus `len_utf8` * `str::char_at` - use slicing plus `chars()` * `str::char_at_reverse` - use slicing plus `chars().rev()` * `str::slice_shift_char` - use `chars()` plus `Chars::as_str` * `CommandExt::session_leader` - use `before_exec` instead. Closes #27719 cc #27751 (deprecating the `Slice` bits) Closes #27754 Closes #27780 Closes #27809 Closes #27811 Closes #27830 Closes #28050 Closes #29453 Closes #29791 Closes #29935 Closes #30014 Closes #30752 Closes #31262 cc #31398 (still need to deal with `before_exec`) Closes #31405 Closes #31572 Closes #31755 Closes #31756

This commit applies all stabilizations, renamings, and deprecations that the library team has decided on for the upcoming 1.9 release. All tracking issues have gone through a cycle-long "final comment period" and the specific APIs stabilized/deprecated are: Stable * `std::panic` * `std::panic::catch_unwind` (renamed from `recover`) * `std::panic::resume_unwind` (renamed from `propagate`) * `std::panic::AssertUnwindSafe` (renamed from `AssertRecoverSafe`) * `std::panic::UnwindSafe` (renamed from `RecoverSafe`) * `str::is_char_boundary` * `<*const T>::as_ref` * `<*mut T>::as_ref` * `<*mut T>::as_mut` * `AsciiExt::make_ascii_uppercase` * `AsciiExt::make_ascii_lowercase` * `char::decode_utf16` * `char::DecodeUtf16` * `char::DecodeUtf16Error` * `char::DecodeUtf16Error::unpaired_surrogate` * `BTreeSet::take` * `BTreeSet::replace` * `BTreeSet::get` * `HashSet::take` * `HashSet::replace` * `HashSet::get` * `OsString::with_capacity` * `OsString::clear` * `OsString::capacity` * `OsString::reserve` * `OsString::reserve_exact` * `OsStr::is_empty` * `OsStr::len` * `std::os::unix::thread` * `RawPthread` * `JoinHandleExt` * `JoinHandleExt::as_pthread_t` * `JoinHandleExt::into_pthread_t` * `HashSet::hasher` * `HashMap::hasher` * `CommandExt::exec` * `File::try_clone` * `SocketAddr::set_ip` * `SocketAddr::set_port` * `SocketAddrV4::set_ip` * `SocketAddrV4::set_port` * `SocketAddrV6::set_ip` * `SocketAddrV6::set_port` * `SocketAddrV6::set_flowinfo` * `SocketAddrV6::set_scope_id` * `<[T]>::copy_from_slice` * `ptr::read_volatile` * `ptr::write_volatile` * The `#[deprecated]` attribute * `OpenOptions::create_new` Deprecated * `std::raw::Slice` - use raw parts of `slice` module instead * `std::raw::Repr` - use raw parts of `slice` module instead * `str::char_range_at` - use slicing plus `chars()` plus `len_utf8` * `str::char_range_at_reverse` - use slicing plus `chars().rev()` plus `len_utf8` * `str::char_at` - use slicing plus `chars()` * `str::char_at_reverse` - use slicing plus `chars().rev()` * `str::slice_shift_char` - use `chars()` plus `Chars::as_str` * `CommandExt::session_leader` - use `before_exec` instead. Closes rust-lang#27719 cc rust-lang#27751 (deprecating the `Slice` bits) Closes rust-lang#27754 Closes rust-lang#27780 Closes rust-lang#27809 Closes rust-lang#27811 Closes rust-lang#27830 Closes rust-lang#28050 Closes rust-lang#29453 Closes rust-lang#29791 Closes rust-lang#29935 Closes rust-lang#30014 Closes rust-lang#30752 Closes rust-lang#31262 cc rust-lang#31398 (still need to deal with `before_exec`) Closes rust-lang#31405 Closes rust-lang#31572 Closes rust-lang#31755 Closes rust-lang#31756

Introduced in Rust 1.8.0 Rust tracking issue: rust-lang/rust#31398 Closes #1

luser · 2016-10-06T12:58:10Z

Is it just the list of issues in your previous comment that need to be sorted out still? I've run into two cases recently where this API would have made my life easier (enabling ptrace for a child process with PTRACE_TRACEME, and getting the mach task for a child process on OS X).

alexcrichton · 2016-10-06T17:09:15Z

@luser yeah most of those are still relevant (I'll update the top comment), unfortunately. It could just be time though to declare "this is the way it'll be"

alexcrichton · 2016-11-01T23:14:12Z

@rfcbot fcp merge

I personally feel that despite the drawbacks before_exec is still useful in situations like @luser mentioned plus others. Although it can be built out-of-tree with CommandExt::exec it's massively painful to do so.

rfcbot · 2016-11-01T23:48:53Z

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.

luser · 2016-11-02T01:55:12Z

FWIW I implemented the ptrace case as a crate here using before_exec: https://github.com/luser/spawn-ptrace

l0kod · 2016-11-06T14:49:22Z

FYI tty-rs use before_exec for setsid(2).

mattgreen · 2016-11-09T22:45:55Z

Looking forward to this landing in stable soon.

Most users don't need it, but when you do, it's really useful. watchexec uses it to create a new process group when launching processes on non-Windows OSes.

rfcbot · 2016-11-12T10:36:31Z

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

psst @alexcrichton, I wasn't able to add the final-comment-period label, please do so.

rfcbot · 2016-11-22T11:07:21Z

The final comment period is now complete.

Library stabilizations/deprecations for 1.15 release Stabilized: - `std::iter::Iterator::{min_by, max_by}` - `std::os::*::fs::FileExt` - `std::sync::atomic::Atomic*::{get_mut, into_inner}` - `std::vec::IntoIter::{as_slice, as_mut_slice}` - `std::sync::mpsc::Receiver::try_iter` - `std::os::unix::process::CommandExt::before_exec` - `std::rc::Rc::{strong_count, weak_count}` - `std::sync::Arc::{strong_count, weak_count}` - `std::char::{encode_utf8, encode_utf16}` - `std::cell::Ref::clone` - `std::io::Take::into_inner` Deprecated: - `std::rc::Rc::{would_unwrap, is_unique}` - `std::cell::RefCell::borrow_state` Closes #23755 Closes #27733 Closes #27746 Closes #27784 Closes #28356 Closes #31398 Closes #34931 Closes #35601 Closes #35603 Closes #35918 Closes #36105

alexcrichton added B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Feb 4, 2016

alexcrichton mentioned this issue Feb 4, 2016

Implement CommandExt::{exec, before_exec} #31409

Merged

alexcrichton mentioned this issue Feb 11, 2016

Expand the scope of std::process rust-lang/rfcs#941

Closed

7 tasks

alexcrichton added B-unstable Blocker: Implemented in the nightly compiler and unstable. and removed B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. labels Feb 18, 2016

alexcrichton added the I-nominated label Mar 9, 2016

alexcrichton mentioned this issue Apr 7, 2016

std: Stabilize APIs for the 1.9 release #32804

Merged

alexcrichton removed the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Apr 13, 2016

alexcrichton changed the title ~~Tracking issue for CommandExt::{exec, before_exec}~~ Tracking issue for CommandExt::before_exec Apr 13, 2016

l0kod pushed a commit to stemjail/tty-rs that referenced this issue Oct 1, 2016

tty: Use before_exec instead of session_leader

8c46173

Introduced in Rust 1.8.0 Rust tracking issue: rust-lang/rust#31398 Closes #1

alexcrichton added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Nov 12, 2016

aturon mentioned this issue Dec 14, 2016

Library stabilizations/deprecations for 1.15 release #38369

Merged

bors closed this as completed in #38369 Dec 18, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tracking issue for CommandExt::before_exec #31398

Tracking issue for CommandExt::before_exec #31398

alexcrichton commented Feb 4, 2016 •

edited

Loading

dirk commented Feb 4, 2016

alexcrichton commented Feb 4, 2016

dirk commented Feb 4, 2016

pnkfelix commented Mar 9, 2016

alexcrichton commented Mar 9, 2016

pnkfelix commented Mar 10, 2016

luser commented Oct 6, 2016

alexcrichton commented Oct 6, 2016

alexcrichton commented Nov 1, 2016

rfcbot commented Nov 1, 2016 •

edited

Loading

luser commented Nov 2, 2016

l0kod commented Nov 6, 2016

mattgreen commented Nov 9, 2016

rfcbot commented Nov 12, 2016

rfcbot commented Nov 22, 2016

Tracking issue for CommandExt::before_exec #31398

Tracking issue for CommandExt::before_exec #31398

Comments

alexcrichton commented Feb 4, 2016 • edited Loading

dirk commented Feb 4, 2016

alexcrichton commented Feb 4, 2016

dirk commented Feb 4, 2016

pnkfelix commented Mar 9, 2016

alexcrichton commented Mar 9, 2016

pnkfelix commented Mar 10, 2016

luser commented Oct 6, 2016

alexcrichton commented Oct 6, 2016

alexcrichton commented Nov 1, 2016

rfcbot commented Nov 1, 2016 • edited Loading

luser commented Nov 2, 2016

l0kod commented Nov 6, 2016

mattgreen commented Nov 9, 2016

rfcbot commented Nov 12, 2016

rfcbot commented Nov 22, 2016

alexcrichton commented Feb 4, 2016 •

edited

Loading

rfcbot commented Nov 1, 2016 •

edited

Loading