signal: misc improvements

[carllerche]

Tracking misc smaller improvements to tokio-signal.

This issue is to be closed once there are no remaining breaking changes to make or remaining breaking changes are tracked in separate issues.

Update to std::future

• follow the future / stream helper strategy from tokio: provide future / stream / sink helpers #1225.

Polish

• replace crate::ctrl_c{,_with_handle} with a CtrlC struct with CtrlC::{new, with_handle} methods (signal: Replace ctrl_c with a CtrlC struct #1273)
• replace crate::windows::Event with crate::CtrlC and crate::windows::CtrlBreak (signal: replace windows::Event with windows::CtrlBreak #1331)
• change crate::unix::Signal::new to accept a new SignalType enum (signal: Add SignalKind for registering signals more easily #1430)
  • this enum should represent all the common signal types (minus the obviously forbidden ones from signal_hook)
  • this enum should support a Raw(c_int) variant to allow specifying a signal using the libc definition (this unblocks anyone who wants to work with an OS specific signal which isn't defined in the SignalType enum)
  • this enum should be marked as nonexhaustive to avoid needing a breaking change to add new signal definitions (tokio_signal should be the only crate that ever needs to match on the types)
• remove libc re-exports in favor of SignalType (signal: Add SignalKind for registering signals more easily #1430)
• consider changing crate::unix::Signal to return () instead of the signum (signal: change unix::Signal to return () instead of signum #1330)
  • It may be useful to instead return the SignalType used during registration in case the caller wishes to listen to multiple signal streams and join them via futures::select
  • However, it is also entirely possible that the caller can simply do Signal::new(sig_type).map(|()| sig_type) if they wish to get this functionality, which keeps our API surface smaller
• Make all CtrlC/CtrlBreak/Signal regular streams (not IoStream) since once they're up and registered there are not errors that are raised
• Try to make the stream constructors return an io::Result<Self> instead of needing an intermediate init future (signal: Change constructors to return a result instead of lazy future #1340)
  • this may be contingent on successfully registering all signal handlers and intermediate types without needing reactor/executor resources
• Remove IoFuture / IoStream definitions
• Bikeshed naming of SignalKind and its associated methods (signal: rename SignalKind methods #1457)

Re-export

• consider re-exporting tokio_signal::CtrlC as tokio::signal::CtrlC (Expose signal from net as a feature #1491)

[ipetkov]

Hey @carllerche thanks for opening this, you've pretty much hit on the major API changes I was intending we update as part of the next breaking milestone (the lack of polish is largely leftover remains from older APIs which couldn't get fixed without breaking changes).

Don't use i32 constants to represent signals (enum, structs, ...)
I was considering changing the futures to return () rather than i32 (and let the caller track what event stream represents what if they need multiple signals). Signals are tightly coupled to the OS/libc, if we expose our own enum for signal types we might need to deal with OS specifics which I'm not sure is going to be worth the effort.

I'd also like to unify the Windows/Unix implementation differences (currently tracked in #1000).

One other idea I wanted to explore is improving the wake performance of signals. Right now all signals get woken each time due to past limitations where dropping the runtime would leak futures. Given this has been fixed I think it's worth revisiting as well.

[ipetkov]

Assigning this to myself, but for anyone interested in tackling any of this, feel free to ping me for questions/help or cc me on any opened PRs!

[carllerche]

@ipetkov cool! Also, just to be explicit. All the points in this issue are up for discussion. I just opened the issue with anything I thought of skimming the code. If you think they are all good ideas, then great 👍

[ipetkov]

I am proposing that we only export CtrlC via tokio::signal for now because it is the only API which is truly cross platform. The remainder of the APIs can be consumed directly through the new tokio-net crate via tokio-net::signal.

Thoughts from @carllerche or anyone else?

[naftulikay]

As a user I'd like to be able to catch and respond to a number of different signal types. I know that most of all of them are Unix specific, but I have an interest in being able to use this for general purpose signal catching.

[ipetkov]

@naftulikay signals are very much a Unix-specific concept. What do you mean by general purpose signal catching?

You can still register interest for any arbitrary Unix signal by using the platform specific APIs defined in the tokio-net crate. For now the consideration is to only reexport the truly cross platform APIs within the tokio crate for ease of use

[naftulikay]

I'm writing a process supervisor which needs to listen for term, quit, and chld signals. Being able to do that with Tokio is great for me as a user, as I get all the benefits of Tokio.

Why are process signals in tokio::net?

[ipetkov]

Why are process signals in tokio::net?

We decided to consolidate various crates together to make maintaining and releasing crates more streamlined (see #1264 for more info). tokio-net happens to include all the reactor and other internals needed for receiving and reacting to signals

[naftulikay]

My apologies: I'm looking through the 0.1 and 0.2 docs and not seeing anything in tokio::net. I'm mainly interested in understanding how the new API will work.

[ipetkov]

The internals will be the same, they're just migrating to where they're defined!
Unfortunately the alpha docs aren't being published on docs.rs, there's a tracking issue for fixing that in #1467

[carllerche]

@ipetkov re: your proposal, that sounds good. My gut feeling is tokio::signal::ctrl_c() -> CtrlC feels more natural than tokio::signal::CtrlC::new().

Edit: the same reasoning as here applies.

[ipetkov]

@carllerche yep that makes sense, I'll take a pass on updating the APIs to remove types where possible, and switch to using a dedicated constructor function rather than ::new()

[ipetkov]

I believe all outstanding issues here have been resolved so I'm going to close this!