smol-rs / async-signal

Asynchronous signal handling
Apache License 2.0
15 stars 6 forks source link

[Question]: why use socket rather rather a channel #36

Closed SteveLauC closed 3 months ago

SteveLauC commented 3 months ago

Hi, thanks for making this neat crate!

While reading the source code, I found that it is using a socket pair as the notifier, I am curious why not use a channel, like Tokio does. Thanks!

notgull commented 3 months ago

It appears that tokio uses a socket pair as well.

The reason why is because its impossible to use an async channel in a signal handler, at least in the general case. Waker can run general user code, and general user code can be non-signal safe.

SteveLauC commented 3 months ago

Hi, thanks for your reply!

It appears that tokio uses a socket pair as well.

I just took a closer look at Tokio's implementation, it seems more complex than I originally thought. Tokio uses both mio::net::UnixStream and tokio::sync::watch, every signal has an atomic flag indicating whether this signal has been sent (i.e., pending state), the the signal handler it registers does 2 things:

  1. Mark the atomic flag so that the signal is pending (NOTE: it is just pending, the corresponding task won't be notified for now)
  2. Write one byte to the tx end of the UnixStream, so that the Driver can be woke up

Once the Driver is woken, it will broadcast the pending signals, i.e., scan all the atomic flags to check if there is any signal that is in the pending state, if so, send a message (()) to the tx end of the corresponding tokio::sync::watch::Sender<()>. Currently, I don't quite understand the purpose behind this 2-layer design.


The reason why is because its impossible to use an async channel in a signal handler, at least in the general case.

Do you mean we cannot call an async function in the sync signal handler? If tx.send() is not async, I guess it would work?

Waker can run general user code, and general user code can be non-signal safe.

Signal-safety is hard, and actually I am struggling with it. Context: I am trying to build the signal futures provided by tokio::signal using event-listener so that I can use them in monoio or other non-Tokio runtimes (Since Tokio signal futures use its io::Driver, it won't work on other runtimes.), looks like there is still something that is not signal-safe in my code, I can get deadlocks sometimes when running unit tests.

notgull commented 3 months ago

Do you mean we cannot call an async function in the sync signal handler? If tx.send() is not async, I guess it would work?

The set of functions that can be called inside of a signal handler is quite small. See this manpage for a list of them.

Signal-safety is hard, and actually I am struggling with it. Context: I am trying to build the signal futures provided by tokio::signal using event-listener so that I can use them in monoio or other non-Tokio runtimes (Since Tokio signal futures use its io::Driver, it won't work on other runtimes.), looks like there is still something that is not signal-safe in my code, I can get deadlocks sometimes when running unit tests.

It should be impossible from the public API exposed by this crate to call any functions in a signal handler that are signal-unsafe. This could be an issue with event-listener, I would appreciate it if you opened an issue on that repository.

SteveLauC commented 3 months ago

The reason why is because its impossible to use an async channel in a signal handler, at least in the general case.

The set of functions that can be called inside of a signal handler is quite small. See this manpage for a list of them.

I guess you are saying that an async channel may allocate? And that's why this crate uses socket and ctrlc uses pipe.

Currently, I don't quite understand the purpose behind this 2-layer design

And, I kinda think this is also the reason behind Tokio's design.

SteveLauC commented 3 months ago

Close as the question has been answered.