-
Notifications
You must be signed in to change notification settings - Fork 73
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 wait_for_shutdown_signal convenience function. #156
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
❗ 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
|
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:
|
Hi vorner,
Ok. I restored Cargo.lock.
The PR adds
I expected this, but the docs didn't say it and the code does not signal-hook/signal-hook-async-std/src/lib.rs Lines 35 to 49 in afa5feb
signal-hook/signal-hook-tokio/src/lib.rs Lines 41 to 61 in afa5feb
It would be good to clarify this, in a separate PR. I removed the
The existing tests use sleeps:
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:
Can you think of a way to write this test without the sleep?
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:
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.
The blocking version
Would you please elaborate on this? Thanks again for making the Sincerely, |
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.
It's on the DeliveryState in src/iterator/backend.rs. The
I'm not against using
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
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 |
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