smol-rs / async-process

Async interface for working with processes
Apache License 2.0
177 stars 29 forks source link

Use a more efficient strategy for waiting on processes #49

Open notgull opened 1 year ago

notgull commented 1 year ago

Right now, we use SIGPROC to wait for process events on Unix and thread pooling to wait for process events on Windows. This is a very inefficient strategy that doesn't scale for many child processes, like using poll() for I/O notification.

Most operating systems have better ways of handling events like this.

mamaicode commented 1 year ago

Will look into this

notgull commented 1 year ago

How this would probably be done:

mamaicode commented 11 months ago

Sorry for the delay! Will start working on this tomorrow

notgull commented 11 months ago

@mamaicode Great! Let me know if you need any guidance; I can hop in a Matrix/Discord call for pair programming if needed.

notgull commented 6 months ago

pidfd has been implemented for Linux. However, we should also be able to implement this for Windows and BSD as well.

rogercoll commented 1 month ago

I was looking to contribute the wait implementation for BSD systems but after giving it a try with the async-io crate I think I would appreciate some guidance. My initial idea was to use the Exit structure to wrap the child process and rely on the async-io reactor for the async registration/poll. This works fine, but the main issue is the Exit structure takes ownership of the Child structure, and as it does not implement Clone/Copy we cannot use it for retrieving the exit code with a try_wait() call. The Exit code and a mutable reference to the child structure is required based on the current crate's api.

@notgull Any idea of how we could solve this issue? The async-io crate registers the process to wait for using the Process structure which does not own the Child struct, instead it relies on its pid and a Child reference. Would it be feasible to add a new/modify Exit structure in async-io that does not take ownership of the Child structure?

notgull commented 1 month ago

I think this could be solved by giving the Exit struct an unsafe method that lets you take a mutable reference to the inner child, then wrapping that in async-process.

rogercoll commented 1 month ago

I think this could be solved by giving the Exit struct an unsafe method that lets you take a mutable reference to the inner child, then wrapping that in async-process.

The thing is that the Child ownership in the Exit struct is given to the Registration struct when its registred in the Reactor:

impl QueueableSealed for Exit {
    fn registration(&mut self) -> Registration {
        Registration::Process(self.0.take().expect("Cannot reregister child"))
    }
}

https://github.com/smol-rs/async-io/blob/master/src/os/kqueue.rs#L252

notgull commented 1 month ago

I think the solution here is to use rustix::process::Pid::from_child to get the PID of the child process, and modify Registration to use that instead.