-
Notifications
You must be signed in to change notification settings - Fork 102
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
Ensure some spawns have deadlines #894
Conversation
424ef01
to
e2187bb
Compare
Signed-off-by: Benjamin Leggett <benjamin.leggett@solo.io>
Signed-off-by: Benjamin Leggett <benjamin.leggett@solo.io>
Signed-off-by: Benjamin Leggett <benjamin.leggett@solo.io>
e2187bb
to
e7a03e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
let drain = std::pin::Pin::new(&mut server); | ||
drain.graceful_shutdown(); | ||
server.await | ||
// There are scenarios where the http2 server never resolves after | ||
// `graceful_shutdown`, which will hang the whole task. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. So before inpod
, drain was the entire process shutting down. Now its not, so it MUST terminate in some bounded time.
Do we want this here, or at the inpod
layer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want this here, or at the inpod layer?
I'm not sure where else to put it, but in terms of error reporting this seems best - this task is spawned to run to completion, and the inpod layer already initiates the drain and waits for it, the most it can do is drop the drain.
(we could do spawnhandles and aborts and punch this up a layer I guess, but the effect would be the same)
This reverts commit e7a03e3.
/retest |
TLS handshakes (tokio-rustls) should have a deadline (practically not a big deal for us, but still)
During certain scenarios where workloads are removed, inbound server "graceful shutdown" can deadlock while
await
ing http2 server to finish up, leaving the spawned task to hang indefinitely, so add a deadline to that as a safety measure (still looking into why it does, theshutdown
part ofgraceful_shutdown
implies a guaranteed termination, but deadline seems safer regardless)1.2.0
(if this is controversial, can drop it)tokio-console
stuff (don't find it terribly useful, also our logrates @ tracelevel tend to overwhelm it, will keep if someone feels strongly)