Add wait_for_shutdown_signal convenience function.

[mleonhard]

Hi vorner,
I started adding a wait_for_shutdown_signal function to https://crates.io/crates/permit . Then I thought that it would probably be cleaner to put it into signal-hook. What do you think?

This PR also deletes Cargo.lock to make CI use the latest versions of dependencies.

Cheers,
mleonhard

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 90.90909% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 62.81%. Comparing base (dd6ccc7) to head (9e77727).
Report is 11 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #156      +/-   ##
==========================================
+ Coverage   60.53%   62.81%   +2.27%     
==========================================
  Files          16       17       +1     
  Lines         778      917     +139     
  Branches      121      141      +20     
==========================================
+ Hits          471      576     +105     
- Misses        237      254      +17     
- Partials       70       87      +17     

Files	Coverage Δ
signal-hook-async-std/src/lib.rs	100.00% <100.00%> (ø)
signal-hook-tokio/src/lib.rs	100.00% <100.00%> (ø)
src/lib.rs	6.60% <85.71%> (+5.59%)	⬆️

... and 7 files with indirect coverage changes

[vorner]

First of, I prefer to keep the Cargo.lock in the repo. I prefer to update manually. Don't worry, the downstream crates don't consider Cargo.lock from libraries, it's only from development ‒ I've had some trouble with the CI updating on its own.

As for the pull request. The general idea seems fine, with some ideas for improvement:

• I wonder why just one of the crates has a feature for it and not the others.
• Generally, the close is not necessary. It is implicit when the instance is dropped.
• I don't think it's a good idea to rely on CI running fast enough, therefore checking some specific times.
• I'd suggest putting the list of signals into a constant somewhere, so it's shared between all the crates.
• I'm not sure if the base case (blocking) version is particularly useful.
• I'm not sure if it needs so heavy-weight privitive as the Signals.

[mleonhard]

Hi vorner,
Thanks for reviewing the PR! 😄

First of, I prefer to keep the Cargo.lock in the repo. I prefer to update manually. Don't worry, the downstream crates don't consider Cargo.lock from libraries, it's only from development ‒ I've had some trouble with the CI updating on its own.

Ok. I restored Cargo.lock.

As for the pull request. The general idea seems fine, with some ideas for improvement:

• I wonder why just one of the crates has a feature for it and not the others.

The PR adds signal_hook_tokio::wait_for_shutdown_signal() which depends on using signal_hook_tokio::SignalsInfo as an async iterator. SignalsInfo implements the async iterator from the futures project, not Tokio's own version. The crate has an futures-v0_3 feature which enables a dependency on futures-core-0_3 which is an alias for crate futures-core. This feature enables the async iterator. I don't understand why the crate uses futures and aliases the dependencies. I want to change it to use Tokio's async iterator and not use anything from futures, but that would be a separate PR. Anyway, wait_for_shutdown_signal() needs the rest of the code for the async iterator, which is in futures-util. Rather than add a hard dependency on that, I added another feature to enable that dependency.

• Generally, the close is not necessary. It is implicit when the instance is dropped.

I expected this, but the docs didn't say it and the code does not impl Drop. Also the examples say to close:

signal-hook/signal-hook-async-std/src/lib.rs

Lines 35 to 49 in afa5feb

	//! #[async_std::main]
	//! async fn main() -> Result<(), Error> {
	//! let signals = Signals::new(&[SIGHUP, SIGTERM, SIGINT, SIGQUIT])?;
	//! let handle = signals.handle();
	//!
	//! let signals_task = async_std::task::spawn(handle_signals(signals));
	//!
	//! // Execute your main program logic
	//!
	//! // Terminate the signal stream.
	//! handle.close();
	//! signals_task.await;
	//!
	//! Ok(())
	//! }

signal-hook/signal-hook-tokio/src/lib.rs

Lines 41 to 61 in afa5feb

	//! #[tokio::main]
	//! # pub
	//! async fn main() -> Result<(), Error> {
	//! let signals = Signals::new(&[
	//! SIGHUP,
	//! SIGTERM,
	//! SIGINT,
	//! SIGQUIT,
	//! ])?;
	//! let handle = signals.handle();
	//!
	//! let signals_task = tokio::spawn(handle_signals(signals));
	//!
	//! // Execute your main program logic
	//!
	//! // Terminate the signal stream.
	//! handle.close();
	//! signals_task.await?;
	//!
	//! Ok(())
	//! }

It would be good to clarify this, in a separate PR.

I removed the close calls.

• I don't think it's a good idea to rely on CI running fast enough, therefore checking some specific times.

The existing tests use sleeps:

• signal-hook/tests/iterator.rs

  Line 70 in afa5feb

  thread::sleep(Duration::from_millis(100));

• signal-hook/signal-hook-async-std/tests/async_std.rs

  Line 48 in afa5feb

  async_std::task::sleep(Duration::from_millis(100)).await;

• signal-hook/signal-hook-tokio/tests/tests.rs

  Line 50 in afa5feb

  tokio::time::sleep(Duration::from_millis(100)).await;

• signal-hook/signal-hook-mio/tests/mio_0_8.rs

  Line 101 in afa5feb

  thread::sleep(Duration::from_millis(25));

We need to test that the function actually waits for the signal. I couldn't think of another way to write the test. I've had good luck with these kinds of tests in my crates, running on Github and Gitlab CI:

• https://gitlab.com/leonhard-llc/ops/-/blob/d8514c5df9949582b06eb458f614402f738da649/permit/tests/test.rs
• https://gitlab.com/leonhard-llc/safina-rs/-/blob/72a26453c5c69d5f9f5cdb709ee6bc8c85a7f000/safina-sync/tests/mutex.rs
• https://gitlab.com/leonhard-llc/safina-rs/-/blob/72a26453c5c69d5f9f5cdb709ee6bc8c85a7f000/safina-sync/tests/channel.rs
• https://gitlab.com/leonhard-llc/safina-rs/-/blob/72a26453c5c69d5f9f5cdb709ee6bc8c85a7f000/safina-timer/src/lib_tests.rs
• https://gitlab.com/leonhard-llc/safina-rs/-/blob/72a26453c5c69d5f9f5cdb709ee6bc8c85a7f000/safina-timer/src/sleep_future_tests.rs
• https://github.com/mleonhard/servlin/blob/d8fed106d1f9202cc560a0143d7dbb8826444a3d/tests/server.rs#L23
• https://github.com/mleonhard/servlin/blob/d8fed106d1f9202cc560a0143d7dbb8826444a3d/tests/handler.rs#L305

Can you think of a way to write this test without the sleep?

• I'd suggest putting the list of signals into a constant somewhere, so it's shared between all the crates.

Would you please share your motivations for this change? I can think of two motivations for doing something similar, but neither benefit us in this PR:

1. If we change the list, we may forgot to change one copy. By putting the list values in a single place, we eliminate this error. In this case, I expect we will never change this list. The list has remained stable for 40 years.
2. If the list is long or confusing, having it in one place reduces the volume of the code. In this case, the list is very tiny and straightforward.

Moving the list to another crate makes it very hard for readers to find the value. For example, when viewing the source code on docs.rs, one cannot click through to the source from the other crate. Navigating to the definition will be slow. So we speed up future readers a lot by putting the list in-line.

• I'm not sure if the base case (blocking) version is particularly useful.

The blocking version signal_hook::wait_for_shutdown_signal() is the one I want. I want to use it in my own prod servers (backends for https://www.applin.dev and https://www.cozydate.com ) and in the example docs for the permit crate: https://docs.rs/permit/latest/permit/#example .

• I'm not sure if it needs so heavy-weight privitive as the Signals.

Would you please elaborate on this?

Thanks again for making the signal-hook crate and for reviewing this PR!

Sincerely,
Michael

[vorner]

I don't understand why the crate uses futures and aliases the dependencies.

If I remember correctly (it's a long time ago), it was in times when tokio didn't have any streaming support yet. I suspect some kind of cleanup and moving to using that might be a good idea some day.

I expected this, but the docs didn't say it and the code does not impl Drop. Also the examples say to close:

It's on the DeliveryState in src/iterator/backend.rs.

The close is so that, when one thread is blocked on the iterator, another thread can remotely terminate the iterator and unblock it. It is used in the examples in such a way.

The existing tests use sleeps:

I'm not against using sleep in the tests. However, in the past, I had some bad experience on relying on a test running fast enough ‒ because the CI machine was severely overloaded. It was generating false failures, because something didn't happen in time. If you look at the existing sleep tests, it checks that something doesn't happen. On a really slow CI, it could cause a false pass, not false failure, which is AFAIK better.

Would you please share your motivations for this change?

Well, one is that I don't like duplicated code.

But as you suggest, it is a commonly used list of signals and a user might find it useful for other purposes. So putting it into the base signal-hook crate, it is available both to our own use but also for a user.

Would you please elaborate on this?

Well, the primitive contains a lot of code to make sure it can make a distinction between different signals. It's complicated because of that and relies on a pipe(like) combined with an array of atomic flags. The application then can consume the signals and dispatch different actions based on what comes, possibly including extended info.

The wait_for_shutdown_signal kind of throws all that out of the window and just… uses the same action for all. We need only a wakeup (unblocking). Which can be done either by a semaphore (sem_post is allowed in a signal handler) or by a pipe(like) primitive alone. The crate provides the pipe primitive ‒ so what you can do is register the same write end to multiple signals and then wait in a blocking wait to read a byte out of it. Now looking at what the crate offers, it seems not to offer a way to directly register the same write end multiple times ‒ so it would have to be duped in some way or provide an abstraction for it.