Tokio: process: misc improvements

[ipetkov]

Tracking misc smaller improvements to tokio-process.

This issue is to be closed once there are no remaining breaking changes to make or remaining breaking changes are tracked in separate issues.

Update to std::future

• [x] follow the future / stream helper strategy from #1225.

Polish

• [x] replace the CommandExt trait in favor of a process::Command wrapper (similar to what tokio-fs does) (#1448)
• [x] remove all blocking methods (#1428)
• [ ] consider allowing a process to be killed through a separate handle rather than just the Child future
• [ ] Update CHANGELOG with all changes since the previous version
• [x] review dependencies.

Re-export

• [ ] consider re-exporting tokio_process::CommandExt as tokio::process::CommandExt

[ipetkov]

Assigning this to myself to review/drive, but any tasks are up for grabs by anyone interested! (feel free to ping me for reviews or ideas!)

[carllerche]

I would recommend avoiding the std::Command / CommandExt strategy and instead provide tokio_process::Command(std::Command) and not expose any blocking fns.

This is the strategy we take in tokio-fs.

[ipetkov]

@carllerche makes sense to me! It will also give us a smaller API surface we can evolve as needed (in retrospect trying to make the async command fit all of the std::process::Command APIs/behaviors was a bit restricting).

[asonix]

Can I jump in and comment on tokio-process usage? It's a little awkward right now for handling an ending process.

Right now, you can kill a child, _or_ wait on the child to finish on it's own. You can't interact with a process that may end on it's own but can also be killed. This is because you need the Child to kill it, but you need to await the Child to see when it finishes.

If there was another concept, like a Handle, that could be used to send signals to the process (more than just Kill would be nice) without needing to keep Child around, it would make this possible.

Edit: here's a way it could look.

let handle = child.signal_handle();
tokio::spawn(async move {
  let _ = child.await;
  // do whatever after process finishes
});
tokio::spawn(async move {
  something.await; // react to some event
  handle.send_signal(Signal::Kill).await;
});

[reiver-dev]

Unfortunately, because async Command does not share anything with std::process::Command anymore, unix-specific capabilities (uid, gid, pre_exec, exec) are now unavailable.

Also it how complicated will it be to allow to receive completion notifications from Handle/Pid created by other means (i.e. direct use of fork, external libs)?

[carllerche]

@reiver-dev we can add unix specific extensions as well.

[fenhl]

I am working on a pull request that will implement std::os::{unix,windows}::process::CommandExt for tokio_process::Command.

[fenhl]

Nevermind, I can't seem to get my fork to compile with the process feature. Someone else will need to take this.

[fenhl]

Got it to compile but I overlooked the problem of CommandExt methods returning &mut Command instead of &mut Self. Maybe a From<StdCommand> impl would make sense here.

[ipetkov]

@fenhl thanks for working on this! I think we should just mirror the CommandExt trait ourselves (and make it return tokio_net::process::Command, but also omit the deprecated meethods). Given that the std version is stabilized, we shouldn't see too much churn on keeping our own version of it. See comment below

[carllerche]

Instead of providing our own CommandExt, I believe we should just add those fns directly on Command scoped by platform.

I believe that the os specific trait *Ext pattern is being phased out. See this blog post. Mio and Tokio will be moving to the pattern.

[ipetkov]

Good to know, that sounds good to me!

[ipetkov]

@reiver-dev

Unfortunately, because async Command does not share anything with std::process::Command anymore, unix-specific capabilities (uid, gid, pre_exec, exec) are now unavailable.

Extensions have now been added!

Also it how complicated will it be to allow to receive completion notifications from Handle/Pid created by other means (i.e. direct use of fork, external libs)?

We rely on std::process::Child to do some checks/guarding for us (e.g. you can't kill the process after it has been reaped because it could be killing another process that was spawned later). It wouldn't be impossible for us to support notifications on externally spawned processes, but it does give us less control to avoid problems like deadlocks or trying to reap zombie processes as a result of dropped handles etc. Plus there are some platform specific differences between Windows and Unix which would also have to be considered...

Do you have a more specific use case for what functionality you're looking for?

[reiver-dev]

Extensions have now been added!

Great! Thank you!

What is your opinion on making exec also available for unix? Technically it is not a blocking call, but will allow to choose between exec into new process or performing non-blocking wait while keeping child initialization procedure the same.

Do you have a more specific use case for what functionality you're looking for?

These cases are likely to be platform-specific:

• Handles acquired by calling OpenProcess
• Creating Reaper for PIDs after manually calling fork (i thinks this requires to mask SIGCHLD during forking)
• Processes that are created by code over FFI boundary

I understand that there is no way to keep safety guarantees and also keep it cross-platform. It seems the question moves into defining the scope of public API (i.e. whether to make Reaper public), so user will be able to reuse global state defined in the library. This might conflict with using Jobs on Windows though. So specific cases might require custom watchers.

[ipetkov]

What is your opinion on making exec also available for unix?

Although it's a more advanced/here-be-dragons type of API, I would personally be okay with adding a method which delegates to std::Command since it doesn't cost us much to maintain (plus we don't need to worry about actual async interactions).

I understand that there is no way to keep safety guarantees and also keep it cross-platform. It seems the question moves into defining the scope of public API (i.e. whether to make Reaper public), so user will be able to reuse global state defined in the library

Let me first start by saying that we'd be happy to consider any concrete proposals for API additions if they have sufficient motivation!

My thoughts on the matter are:

• I would be in favor of adding support to unsafely take ownership of a raw pid and treat it the same way as if the process was spawned through our other APIs, though there would be some extra work we would need to do to perform the same types of safety the stdlib does for us
• I would not be in favor of publicly exposing the Reaper, et al, as they are today (at least not without deeper consideration as to how the API will interact with everything else). Right now they're very much an implementation detail which tries to impose a few assumptions on the caller as possible (e.g. the Reaper stops doing anything if there aren't any Child futures being polled)

[ipetkov]

I'm going to resolve this tracking issue since I believe we have all the breaking API changes in the state that we want them for the 0.2 release.

For any outstanding feature requests (which do not require breaking changes), please feel free to open a new issue, and we can track the discussion there!