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

Add a way to run without the async-process thread #52

Merged
merged 5 commits into from
Oct 11, 2023
Merged

Conversation

notgull
Copy link
Member

@notgull notgull commented Sep 23, 2023

I know I said that I wouldn't add any more features, but I think this is important enough.

Right now, a thread called "async-process" is responsible for listening for SIGCHLD and reaping zombie processes. This listens for the SIGCHLD signal in Unix and uses a channel connected to the waitable handle on Windows. While this works, we can do better. Through async-signal, the signal was already asynchronous on Unix; we were already just using async_io::block_on to wait on the signal. After swapping out the channel used on Windows with async-channel, the process reaping function "reap" can be reimplemented as a fully asynchronous future.

From here we must make sure this future is being polled at all times. To facilitate this, a function named "driver()" is added to the public API. This future acquires a lock on the reaper structure and calls the "reap()" future indefinitely. Multiple drivers can be created at once; they will just wait forever on this lock. This future is intended to be spawned onto an executor and left to run forever, making sure all child processes are signalled whenever necessary. If no tasks are running the driver future, the "async-process" thread is spawned and runs the "reap()" future itself.

I've added the following controls to make sure that this system is robust:

  • If a "driver" task is dropped, another "driver" task will acquire the lock and keep the reaper active.
  • Before being dropped, the task checks to see if it is the last driver. If it is, it will spawn the "async-process" thread to be the driver.
  • When a Child is being created, it checks if there are any active drivers. If there are none, it spawns the "async-process" thread itself.
  • One concern is that the driver future wil try to spawn the "async-process" thread as the application exits and the task is being dropped, which will be unnecessary and lead to slower shutdowns. To prevent this, the future checks to see if there are any extant Child instances (a new refcount is added to Reaper to facilitate this). If there are none, and if there are no zombie processes, it does not spawn the additional thread.
  • Someone can still mem::forget() the driver thread. This does not lead to undefined behavior and just leads to processes being left dangling. At this point they're asking for wacky behavior.

This strategy might also be viable for async-io, if we want to try to avoid needing to spawn the additional thread there as well.

Closes #7
cc smol-rs/async-io#40

I know I said that I wouldn't add any more features, but I
think this is important enough.

Right now, a thread called "async-process" is responsible for listening
for SIGCHLD and reaping zombie processes. This listens for the SIGCHLD
signal in Unix and uses a channel connected to the waitable handle on
Windows. While this works, we can do better. Through async-signal, the
signal was already asynchronous on Unix; we were already just using
async_io::block_on to wait on the signal. After swapping out the channel
used on Windows with async-channel, the process reaping function "reap"
can be reimplemented as a fully asynchronous future.

From here we must make sure this future is being polled at all times. To
facilitate this, a function named "driver()" is added to the public API.
This future acquires a lock on the reaper structure and calls the
"reap()" future indefinitely. Multiple drivers can be created at once;
they will just wait forever on this lock. This future is intended to be
spawned onto an executor and left to run forever, making sure all child
processes are signalled whenever necessary. If no tasks are running the
driver future, the "async-process" thread is spawned and runs the
"reap()" future itself.

I've added the following controls to make sure that this system is
robust:

- If a "driver" task is dropped, another "driver" task will acquire the
  lock and keep the reaper active.
- Before being dropped, the task checks to see if it is the last driver.
  If it is, it will spawn the "async-process" thread to be the driver.
- When a Child is being created, it checks if there are any active
  drivers. If there are none, it spawns the "async-process" thread
  itself.
- One concern is that the driver future wil try to spawn the
  "async-process" thread as the application exits and the task is being
  dropped, which will be unnecessary and lead to slower shutdowns. To
  prevent this, the future checks to see if there are any extant `Child`
  instances (a new refcount is added to Reaper to facilitate this). If
  there are none, and if there are no zombie processes, it does not
  spawn the additional thread.
- Someone can still `mem::forget()` the driver thread. This does not
  lead to undefined behavior and just leads to processes being left
  dangling. At this point they're asking for wacky behavior.

This strategy might also be viable for `async-io`, if we want to try to
avoid needing to spawn the additional thread there as well.

Closes #7
cc smol-rs/async-io#40

Signed-off-by: John Nunley <dev@notgull.net>
Signed-off-by: John Nunley <dev@notgull.net>
Signed-off-by: John Nunley <dev@notgull.net>
@notgull notgull requested a review from fogti October 6, 2023 01:56
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
Signed-off-by: John Nunley <dev@notgull.net>
src/lib.rs Outdated Show resolved Hide resolved
Signed-off-by: John Nunley <dev@notgull.net>
@notgull notgull merged commit f733a83 into master Oct 11, 2023
14 checks passed
@notgull notgull deleted the notgull/driver branch October 11, 2023 00:47
@notgull notgull mentioned this pull request Oct 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

More ligthweight waiting for signals
2 participants