feat(web): allow to shutdown faster when there is no more requests#1193
feat(web): allow to shutdown faster when there is no more requests#1193joelwurtz wants to merge 5 commits into
Conversation
52ce564 to
27dfc69
Compare
| assert_eq!("GET Response", body); | ||
| } | ||
|
|
||
| handle.try_handle()?.stop(false); |
There was a problem hiding this comment.
before this change this would wait for the keep alive timeout, now it shut down instantly (because there is no pending request)
2f236c9 to
91a691c
Compare
|
This implementation works to shutdown faster on h1 / h2 and h3 when there is no more requests, however i'm sure there is a better way to do it |
f2d2069 to
cc92695
Compare
cc92695 to
6a58422
Compare
6a58422 to
755b6c7
Compare
|
Can you leave out the h2 dispatcher part for now? The code base for it is still very messy and some code path surround shutdown handling could be get refactored recently. I can take over that part after I rule out all the obvious bugs. The service and server part are reasonable |
…ages when app shutdown on h2
|
Yeah sure will remove it (but last changes make it works fine). Are you fine with the design of the "ShutdownToken" ? I remove the deps to tokio util to use a "simple" approach, however it requires spin (which gives us a Mutex in a non std env), and it also require the "alloc" feature (to be able to box pin the future). |
|
I'm thinking about a "simpler" design which was partially working with the old http2 implementation. Being the "connection: close" header would cause connection shutdown. In http1 we already have this behavior embeded in Response encoding. The new homebrew http2 dispatcher still lacks this behavior but I can imagine the process of it being: That's it for the dispatcher level. For high level http user I can imagine a cancel token or some channel based middleware after observe a certain conditon would add "connection: close" to all response headers. This would result in a robust graceful shutdown for http2 and http3. You just send a request and then the app would be go away without dropping other potential ongoing concurrent requests. And of course we should allow hooking up cancel token into acceptor types that listening to new streams otherwise new streams may keep coming in. This approach is not as directly(you have to issue one request to immediately trigger the shutdown) as hooking up cancel token to http services but I guess it's more useful in general. What do you think of this approach? |
|
BTW if we are not adding cancel token to Listener types we can ultize ReadyService impl to stop http service from accepting new streams. In xitca-server we always call ReadyService::ready before accepting new streams. So the http services just need to pending and throttle itself after observing cancel token. This can also be achieved with middlware |
|
We can start from this PR. From what I see the cancel token is mostly about cancel the KeepAlive timer. We can expand an arbitrary type into The cancel token can live inside |
|
Thanks for the follow up, i think #1366 is a good start also, i was also wondering if the keep alive can be updated to also support shutdown. However to make it works nicely i think it can be good to also have a specific object which is public an can be shared by users of this library. As an example we work on a reverse proxy, and having something to inform us that the app is in shutdown mode is crucial to allow closing existant connection (like websocket). All of my PR are in a fork actually, and it works nicely, not sure if this is wanted but with the shutdown and some other changes we are actually able to restart the server without loosing new connections Existing connections are still shutdown, but properly, and when there is a new connection during shutdown it's in the accept queue until the new process is restarted, and we need this shutdown to be fast in order to not fill up this queue when migrating the socket from old to new process. |
|
Yes. From the get go we would only offer a default No-op type that can be overridden by API like We can later introduce a specific token implement in That said it would be even more useful if we simply expose a TImer trait that can cover the cancel token's use case and enable possibly runtime independant from |
Ref #1190
Still a draft but this is an example on how we could shutdown faster and cleanly even when there is still pending connections, this is a proof of concept on h1 dispatcher, h2 and h3 can do something similar also
I used a
tokio::sync::watchchanneltokio_util::CancellationTokento check for shutdown change but maybe there is a better solution (there is a lot of things to adapt, like naming, correct deps, etc ... but this is mainly an example for the moment)