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

Expand the scope of std::process #941

Closed
alexcrichton opened this Issue Mar 5, 2015 · 20 comments

Comments

Projects
None yet
@alexcrichton
Copy link
Member

alexcrichton commented Mar 5, 2015

Like #939, this issue is intended to collect information about the expansion of the std::process module. As with most of the rest of the I/O reform, the goal of std::process was originally to be quite conservative. Over time we knew we were going to want to expand the module after we got experience using it as well as seeing how this sort of expansion plays out in the bigger picture.

I'll list some pieces below of parts I think that could be expanded, and I'll try to keep the list updated over time:

  • The I/O handle types do not expose their internals. This means that on unix even though a pipe is known to be a file descriptor, the file descriptor is not accessible for interoperation with other I/O libraries.
  • It is not possible to specify a custom I/O handle as one of the stdio descriptors. For example on unix the stdin could be specified as a specific file descriptor which already exists. This functionality needs to be fleshed out on Windows first. - #893
  • Process spawning should expose dropping capabilities on linux - rust-lang/rust#12137
  • File descriptors may be leaked when spawning processes - rust-lang/rust#12148
  • Spawning a daemon process is not currently possible (on either windows or unix)
  • A post-fork function to execute on unix would be useful for fine-tuning behavior for the child
  • Waiting for a child with a timeout on windows is fairly trivial to implement but is not expose currently. (implemented in a separate repo)

As with #939 I don't plan on writing an RFC about this expansion in the near future, but I'd like one place to collect information about how std::process can be expanded.

@alexcrichton alexcrichton added the A-libs label Mar 5, 2015

alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 5, 2015

std: Deprecate the old_io::process module
This module is now superseded by the `std::process` module. This module still
has some room to expand to get quite back up to parity with the `old_io`
version, and there is a [tracking issue][issue] for feature requests as well as
known room for expansion.

[issue]: rust-lang/rfcs#941
[breaking-change]

Manishearth added a commit to Manishearth/rust that referenced this issue Mar 6, 2015

Rollup merge of rust-lang#23079 - alexcrichton:deprecate-process, r=a…
…turon

 This module is now superseded by the `std::process` module. This module still
has some room to expand to get quite back up to parity with the `old_io`
version, and there is a [tracking issue][issue] for feature requests as well as
known room for expansion.

[issue]: rust-lang/rfcs#941
[breaking-change]
@l0kod

This comment has been minimized.

Copy link

l0kod commented Mar 6, 2015

std::process should provide a way to deliberately leak file descriptors/handles and get/use them back (i.e. old Command::extra_io, lost with rust-lang/rust#18557), not only stdio.

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Mar 6, 2015

Certainly, I consider that to fall under the "It is not possible to specify a custom I/O handle as one of the stdio descriptors" umbrella where we're not really leaking them per-se but you should be able to specify that an existing file descriptor should exist as another in the new process.

@l0kod

This comment has been minimized.

Copy link

l0kod commented Mar 21, 2015

A post-fork function (cf. #579 (comment)) seems to need properties similar to FnAsync (i.e. signal handler) to be safe.

@nagisa

This comment has been minimized.

Copy link
Contributor

nagisa commented May 18, 2015

Need to add a way to wait for children with a timeout.

@genodeftest

This comment has been minimized.

Copy link

genodeftest commented May 29, 2015

I need a way to check whether a child process is still running or not.

@l0kod

This comment has been minimized.

Copy link

l0kod commented Jun 20, 2015

Is there any plan to re-add detached(&mut self) to std::os::unix::process::CommandExt? Is there another way to do it without a manual unsafe fork(2) handling?

cc @aturon

bors added a commit to rust-lang/rust that referenced this issue Aug 4, 2015

Auto merge of #26470 - l0kod:process-session-leader, r=alexcrichton
Add a new method `CommandExt::session_leader(&mut self, on: bool)` to create a new session (cf. `setsid(2)`) for the child process. This means that the child is the leader of a new process group. The parent process remains the child reaper of the new process.

This is not enough to create a daemon process. The *init* process should be the child reaper of a daemon. This can be achieved if the parent process exit. Moreover, a daemon should not have a controlling terminal. To acheive this, a session leader (the child) must spawn another process (the daemon) in the same session.

cc rust-lang/rfcs#941
cc #17176
@pnathan

This comment has been minimized.

Copy link

pnathan commented Oct 11, 2015

Hi,

I think this is the relevant RFC, just let me know if it isn't and I'll remove my comment and file elsewhere.

About 1 year ago, I had a nice piece of code[0], which let me run a subprocess and provide streaming updates of its stdout/stderr descriptors. It is code which is intended for a long-running system: e.g., commands that run for hours and the controlling job needs to respond rather than block.

Right now, Process::Command & family do not appear to support this ability. I think with certain abuses of the borrow checker, I can kludge my way to part of this, but it's a lot of fiddly work when really, the original API supported what I need.

I'd like to see functionality in std::process explicitly for this case.

[0] https://gist.github.com/pnathan/30e283cbcf2bf4f46346#file-action-lib-rs-L134

@BurntSushi

This comment has been minimized.

Copy link
Member

BurntSushi commented Oct 12, 2015

@pnathan I only briefly scanned your code, but why exactly can't you just read from the Child stdout/stderr descriptors?

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Oct 12, 2015

@pnathan I agree with @BurntSushi that what you did previously should definitely be possible with today's std::process. I also updated the checklist above and basically all I/O-related tasks have since been implemented in one form or another.

@pnathan

This comment has been minimized.

Copy link

pnathan commented Oct 12, 2015

@alexcrichton @BurntSushi - hmm. Maybe I got into a corner with reading from the pipes, certainly has happened before. However, I still don't see any way in the API to check if a process is dead, only wait. Am I misunderstanding something?

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Oct 12, 2015

We could pretty easily add a nonblocking version of wait but I don't think we do right now.

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Oct 12, 2015

@pnathan yeah there's not currently a way to do that, but that's largely because it's quite tricky! On Unix unfortunately kill with a 0 signal won't work reliably 100% of the time because there's no guarantee that the pid being verified hasn't been recycled. I think the best way to do this would be wait with WNOHANG, but that unfortunately needs builtin support that @sfackler mentioned.

In the meantime though so long as you're careful with calling wait you can use this repo in the meantime which should allow you to wait with a timeout of 0.

@l0kod

This comment has been minimized.

Copy link

l0kod commented Oct 12, 2015

On Unix unfortunately kill with a 0 signal won't work reliably 100% of the time because there's no guarantee that the pid being verified hasn't been recycled.

On Linux, the PID is only recycled if the parent waits for the child exit code, otherwise it is transform into a zombie process.
In the future, CLONE_FD could land in the kernel, which would be much safer.

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Oct 12, 2015

Ah right of course!

@emilliken

This comment has been minimized.

Copy link

emilliken commented Oct 25, 2015

Another one for the list above- It is not possible to specify a custom I/O handle as one of the NON-stdio descriptors (extra_io)

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Nov 10, 2015

cc #1359, an RFC to add a few extra pieces of functionality on Unix

@LukasKalbertodt

This comment has been minimized.

Copy link
Contributor

LukasKalbertodt commented Jan 31, 2016

What about process groups or child processes of some Child? I don't know how this works with Windows, though. But I guess it should be somehow possible to get access to the process group of one spawned process.

@kamalmarhubi

This comment has been minimized.

Copy link
Contributor

kamalmarhubi commented Feb 1, 2016

With Linux, it'd be good to expose the PR_SET_CHILD_SUBREAPER prctl here. This would make the parent process play the role of init for any processes spawned by the child. Docs at prctl(2).

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Feb 11, 2016

With #1359 now implemented in rust-lang/rust#31409 (and tracked for stabilization by rust-lang/rust#31398) this issue should now basically be entirely possible using libstd itself.

The original motivation has basically now been fulfilled, so I'm going to close this. There are perhaps some more specific APIs which could be added or bound, but those likely want to have separate issues or start out as crates on crates.io.

@kamalmarhubi

This comment has been minimized.

Copy link
Contributor

kamalmarhubi commented Feb 11, 2016

👍

@petrochenkov petrochenkov added the T-libs label Jan 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.