2862: GracefulShutdown utility #86

GlenDC · 2024-01-08T21:50:41Z

Adds a GracefulShutdown utility which:

is runtime agnostic
can work with any future
respects the API as requested in the feature request issue (#2862), taken also into account comments and ideas from this PR;

seanmonstar · 2024-01-10T21:46:34Z

First of all, the work in here looks really nice!

I do not take a generic future but instead implement a trait myself that I implement for the http1 conn, http2 conn and auto conn (when feature is enabled), so that I can do the dance for the user

I think this is probably the best one. You can look at how hyper 0.14.x did it for the internal future logic.

Some other thoughts:

If you remove the Clone impl, then I think you could also remove the Result from the watch() method, since you know it could never have started shutdown on a clone.

GlenDC · 2024-01-10T21:54:23Z

Great to hear @seanmonstar will look to finish this PR for a first non-draft review this week then, based on this feedback.

GlenDC · 2024-01-11T08:14:50Z

@seanmonstar the clone can only be removed if you only ever want one shutdown? Unless you start stacking shutdowns?
I was respecting for now the desired API at hyperium/hyper#2862, is that no longer desired then?

in replacement of futures in generic

GlenDC · 2024-01-11T09:19:53Z

@seanmonstar:

Graceful utility now operates on / watches Connections (http1 / http2 / auto), normal and upgradable, rather then a generic future. This allows us to implement the graceful_shutdown logic for the user;
To keep it as generic as possible I introduced a future parameter in the watch method so that the user can choose how to signal a shutdown rather then us linking it internally to shutdown...
- another option would be to use something like a broadcast channel or I implement a subscriber ourself so we can hook something lightweight automatically up that will trigger based on the shutdown logic... Would add more code to maintain, but maybe more user-friendly. I'll hold off on this until I know what you want to do with this
To be able to gracefully handle connections i do need tokio::select as that's what I've always used
- as such graceful utility now requires tokio:dep, let me know if you do not want this. In the latter case I'll need some feedback/direction on how you would want it instead.

Still to do as well are tests and an example. I'll do this only once you both agree (@seanmonstar and @davidpdrsn) on the current design / implementation. Would rather focus on that and my open questions prior to starting to add testing and examples.

GlenDC · 2024-01-11T21:39:41Z

@seanmonstar and @davidpdrsn I know have a working example: https://github.com/hyperium/hyper-util/blob/8f24314cf040faba15190ff5b75652d3e4c70e52/examples/server_graceful.rs

You can play with it yourself.

Figured that doing the example already would help me iron out some details myself.

I now no longer require Clone and thus the watch function also requires no Result (moved it out into a watcher, which keeps a ref count of itself);
I no longer require the connection / future to be static, but instead to live as long as the watcher. This makes it all work nicely.

Example works and AFAIK the design is now final and ready for your review.

Once you both approve of the design and we worked out some comments you might have, i can finish this off by adding also some unit tests.

this is now handled automatically based on shutdown call

GlenDC · 2024-01-13T22:01:29Z

@seanmonstar and @davidpdrsn , this PR is now ready for "final" review, I am finished.

Updates since last time:

added tests
improved bits and bobs
dropped dependency on tokio, by not using tokio::select
no longer require the cancel func in watch, we now fully shutdown based on the single shutdown call

There's also an example that you can play with:

cargo run --features="tokio server-graceful server-auto" --example server_graceful

Looking forward to the review. As it's my first PR to this organisation I do expect there might be some more back and forward needed. You tell me, as I do think it's ready but 🤷 who knows.

GlenDC · 2024-02-14T20:11:25Z

@seanmonstar is there work to be done here or something I can do to help make this progress. Also if there's something else you have in mind I can look at next, I could take a look at it. The hyper issue seemed interesting but I think I would like to also start contributing a bit to the h2/h3 crates as a way to motivate myself to get more familiar with these.

Anything you have in mind?

seanmonstar · 2024-02-16T14:31:27Z

Sorry I haven't had the time to review again. I was just out for a week escaping the winter.

@programatik29 @dswij @tottoto any of you want to take a look? (Relevant issue and discussion: hyperium/hyper#2862)

src/server/graceful.rs

dswij · 2024-02-27T04:10:20Z

src/server/graceful.rs

+    state: Arc<GracefulState>,
+    // state that the watched futures to know when shutdown signal is received
+    future_state: Arc<GracefulState>,


IIUC, the future_state here is to keep track of all the futures. What does the state do for GracefulShutdown here?

IIUC = If I understand Correctly, didn't know that was a thing.

Naming is hard though. I am essentially using the same concept in two directions.

the "state" is to keep track of all watchers active. When the GracefulShutdown turns into a GracefulWaiter upon self::shutdown() it uses it to check when the counter is 0 and thus when shutdown is finished. While it is still a GracefulShutdown it clones this state and patches it to the GracefulWatchers (the ones watching the "graceful" connections) to help to do its bookkeeping using this counter

the "future_state" is only ever once incremented (it is the counter of the GracefulShutdown itself, of which there is only ever one. And it is used by the "graceful" connections to know if a cancel is triggered. This is essentially "reusing" the GracefulWaiter logic that was already created for the other direction.

So no probably due to the fault of my part (not sure if bad documentation, naming or code) it seemed that you didn not understand it correctly. As it is the state which is there to keep track of all the futures (futures as in graceful connections).

dswij · 2024-02-27T04:14:11Z

src/server/graceful.rs

+    pub fn watcher(&self) -> GracefulWatcher {
+        self.state.subscribe();
+        GracefulWatcher {
+            state: self.state.clone(),
+            future_state: self.future_state.clone(),
+        }
+    }


We can spawn multiple GracefulWatcher from a single GracefulShutdown, so is it accurate to say that the states in GracefulShutdown is for every watcher and futures?

Yes. That is accurate, all these GracefulWatchers lead to the same GracefulShutdown. Once you shutdown however you consume the GracefulShutdown and no new graceful watchers can be created.

dswij · 2024-02-27T04:26:16Z

src/server/graceful.rs

+
+struct GracefulWaiterState {
+    state: Arc<GracefulState>,
+    key: Option<usize>,


What does the key here do?

Is that a question or you want me to comment it? It is the insertion key in the waker list, important for it to be removed again.

dswij · 2024-02-27T04:30:55Z

src/server/graceful.rs

+            .load(std::sync::atomic::Ordering::SeqCst)
+            == 0
+        {
+            // check again in case of race condition


What's the scenario for race condition here?

We first check the counter. That one is zero only if shutdown is already triggered. We then wait for a lock. However it is possible that that we had to wait for the lock while something else was holding the lock to awake the ones waiting for shutdown.

If we do not check again once we have the lock it is possible that the counter in meanwhile is zero. If so we either way need to trigger shutdown here or else we might not be woken up.

This was discovered while developing the origin code on which this PR was inspired (https://github.com/plabayo/tokio-graceful, a crate of mine), where loom helped me to find that issue.

Co-authored-by: dswij <dharmasw@outlook.com>

GlenDC · 2024-03-13T13:48:11Z

Recreated the PR as #108. Sorry for the confusion @seanmonstar and @dswij .

Github does say the vague "bad things will happen" msg when deleting repo, but might be more useful if it can actually say that only and with specifics. As I didn't realise this PR was still attached to that repo. Thought it was a fork from an older idea.

glendc added 2 commits January 8, 2024 22:53

first stab in a runtime agnostic graceful utility

18ec809

add extra check for unwatch/wait-poll race condition

dde7324

seanmonstar requested review from seanmonstar and davidpdrsn January 10, 2024 20:13

implement gracefull connection support

c78a8eb

in replacement of futures in generic

improve design and add graceful server example

8f24314

glendc added 3 commits January 13, 2024 15:04

no longer require cancel fn for graceful future

09e5851

this is now handled automatically based on shutdown call

simplify shutdown code by reusing existing state unsubscribe logic

09c3c05

add graceful tests + drop tokio dependency

8e5db0f

GlenDC marked this pull request as ready for review January 13, 2024 21:58

provide+use GracefulWaiter constructor

6a9044a

dswij reviewed Feb 27, 2024

View reviewed changes

Update src/server/graceful.rs

2fe93bb

Co-authored-by: dswij <dharmasw@outlook.com>

Nugine mentioned this pull request Mar 5, 2024

upgrade hyper and http-body to 1.x Nugine/s3s#134

Merged

plabayo closed this by deleting the head repository Mar 13, 2024

GlenDC mentioned this pull request Mar 13, 2024

#2862: GracefulShutdown utility #108

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2862: GracefulShutdown utility #86

2862: GracefulShutdown utility #86

GlenDC commented Jan 8, 2024 •

edited by seanmonstar

Loading

seanmonstar commented Jan 10, 2024

GlenDC commented Jan 10, 2024

GlenDC commented Jan 11, 2024

GlenDC commented Jan 11, 2024 •

edited

Loading

GlenDC commented Jan 11, 2024

GlenDC commented Jan 13, 2024

GlenDC commented Feb 14, 2024

seanmonstar commented Feb 16, 2024

dswij Feb 27, 2024

GlenDC Feb 27, 2024

GlenDC Feb 27, 2024

dswij Feb 27, 2024

GlenDC Feb 27, 2024 •

edited

Loading

dswij Feb 27, 2024

GlenDC Feb 27, 2024

dswij Feb 27, 2024

GlenDC Feb 27, 2024

GlenDC commented Mar 13, 2024

2862: GracefulShutdown utility #86

2862: GracefulShutdown utility #86

Conversation

GlenDC commented Jan 8, 2024 • edited by seanmonstar Loading

seanmonstar commented Jan 10, 2024

GlenDC commented Jan 10, 2024

GlenDC commented Jan 11, 2024

GlenDC commented Jan 11, 2024 • edited Loading

GlenDC commented Jan 11, 2024

GlenDC commented Jan 13, 2024

GlenDC commented Feb 14, 2024

seanmonstar commented Feb 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GlenDC Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GlenDC commented Mar 13, 2024

GlenDC commented Jan 8, 2024 •

edited by seanmonstar

Loading

GlenDC commented Jan 11, 2024 •

edited

Loading

GlenDC Feb 27, 2024 •

edited

Loading