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

Use a more efficient strategy for waiting on processes #49

Open
notgull opened this issue Aug 20, 2023 · 9 comments
Open

Use a more efficient strategy for waiting on processes #49

notgull opened this issue Aug 20, 2023 · 9 comments

Comments

@notgull
Copy link
Member

notgull commented Aug 20, 2023

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.

  • Linux has pidfd, which can be registered directly into async-io.
  • BSD has EVFILT_PROC, which is now exposed in async-io.
  • Windows has waitable handles, which need to be exposed in async-io. They can also be made more efficient on the polling side, see Support waitable objects on Windows 10 async-io#25
@mamaicode
Copy link

Will look into this

@notgull
Copy link
Member Author

notgull commented Nov 14, 2023

How this would probably be done:

  • Split the signal-handling logic out of Reaper and move it to a signal.rs file.
  • Create an alternative pidfd.rs file that implements the Reaper logic using Async<OwnedFd> and pidfd_open.
  • Child::wait should be implemented by calling readable() on the PIDFD to wait.
  • Zombie processes should be pushed into an executor that replaces the reap() function.

@mamaicode
Copy link

Sorry for the delay!
Will start working on this tomorrow

@notgull
Copy link
Member Author

notgull commented Dec 3, 2023

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

@notgull
Copy link
Member Author

notgull commented Apr 20, 2024

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

@rogercoll
Copy link

rogercoll commented Sep 26, 2024

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
Copy link
Member Author

notgull commented Sep 27, 2024

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
Copy link

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
Copy link
Member Author

notgull commented Oct 6, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants