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.

Sign up for GitHub

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

Jump to bottom

More ligthweight waiting for signals #7

Closed
vorner opened this issue Dec 29, 2020 · 4 comments · Fixed by #52
Closed

More ligthweight waiting for signals #7

vorner opened this issue Dec 29, 2020 · 4 comments · Fixed by #52

Comments

@vorner
Copy link

vorner commented Dec 29, 2020

I've noticed in #4 that the waiting for signals on unix is more heavy-weight than it probably deserves. The signal-hook::iterator::Signals is the most comfortable interface for application usage, as it allows conveniently blocking on an iterator and can handle multiple distinct signals at once, but it is also quite a bit of code that needs to be compiled (it can be turned off by a feature) and needs a separate thread for the blocking. It doesn't seem this crate would need any kind of multiple signals support and blocking is probably even an anti-feature.

I would send a pull request with doing something more lightweight, but I want to discuss design first.

  • The easiest way would be to replace the Signals with pipe. This would allow removing the feature flag and cutting down on the amount of code being compiled.
  • The next step would be to use some FD-thing (pipe/socket) that is async ready and starting a task instead of thread. But that would probably mean the code between unix and windows would diverge a bit further.
  • Or, alternatively, is the wakeup mechanism inside smol async-signal-safe, or does it use mutexes or something like that? If it is safe, it would be possible to just notify from within the signal handler itself. That would need a bit of unsafe (promise that it's really async-signal-safe), but would allow going directly to signal-hook-registry and not creating further file descriptors.

What do you think would be the best way forward?
@taiki-e
Copy link
Collaborator

taiki-e commented Jan 16, 2021

Thanks for the suggestion!

  • The easiest way would be to replace the Signals with pipe. This would allow removing the feature flag and cutting down on the amount of code being compiled.

I haven't investigated how much the compile time changes depending on whether the feature is enabled, but this seems to make sense.

  • The next step would be to use some FD-thing (pipe/socket) that is async ready and starting a task instead of thread. But that would probably mean the code between unix and windows would diverge a bit further.
  • Or, alternatively, is the wakeup mechanism inside smol async-signal-safe, or does it use mutexes or something like that? If it is safe, it would be possible to just notify from within the signal handler itself. That would need a bit of unsafe (promise that it's really async-signal-safe), but would allow going directly to signal-hook-registry and not creating further file descriptors.

cc @stjepang What do you think about this? (I'm not familiar enough to answer/review this.)

@tinywombat765
Copy link

tinywombat765 commented Feb 7, 2021 •
edited
Loading

Not sure if this would impact this issue but i recently had my PR merged that made signal-hook-async-std only use async-io, thereby making it usable with smol with no extra async dependacies.

Edit: and futures-lite. That change is in version 0.2.1.

vorner/signal-hook#91

@voidc
Copy link

voidc commented Feb 7, 2021

Another alternative to signals might by pidfds. There already exists a crate which uses them with async-io: async-pidfd

@vorner
Copy link
Author

vorner commented Feb 7, 2021

signal-hook-async-std would be more heavyweight than what's there right now (at least in the amount of dependencies).

pidfd is probably linux specific, non-portable.

@notgull notgull mentioned this issue Dec 31, 2022
Closed
@notgull notgull mentioned this issue Apr 30, 2023
Merged
notgull added a commit that referenced this issue Sep 23, 2023
@notgull
Add a way to run without the async-process thread
381e7f5 
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>
@notgull notgull mentioned this issue Sep 23, 2023
Merged
notgull added a commit that referenced this issue Oct 11, 2023
@notgull
feat: Add a way to run without the async-process thread
f733a83 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
4 participants
@voidc @tinywombat765 @vorner @taiki-e