server: fix http graceful shutdown #1090
Conversation
| Either::Left((conn, _)) => conn, | ||
| Either::Right((_, mut conn)) => { | ||
| Pin::new(&mut conn).graceful_shutdown(); | ||
| conn.await |
There was a problem hiding this comment.
this is the fix, we need to await for the conn future anyway not just call graceful_shutdown my bad
There was a problem hiding this comment.
There was a problem hiding this comment.
Ah interesting! (I found https://docs.rs/hyper/latest/hyper/server/conn/struct.Connection.html#method.graceful_shutdown more useful)
Might be worth a comment next to these lines to note that since it's a slightly odd API?
tests/tests/integration_tests.rs
Outdated
| // The pending calls should be answered before shutdown. | ||
| assert_eq!(res.await.unwrap(), 10); |
There was a problem hiding this comment.
So If I understand right, the core of this test is that it makes some requests to the server which will take time to respond, and then it asks to stop the server (after all calls have definitely made it to the server but before they will have resolved) and all calls should resolve before the server stops?
Sounds good to me offhand! I assume that without your fix above this fails?
I wonder whether some comment might make it easier to understand the aim, like:
/// - Make 10 calls to the server.
/// - Once they have all reached the server but before they have responded, stop the server.
/// - All calls should be responded to before the server shuts down.
Out of interest what happens if, while it's shutting down (ie while waiting for calls to be responded to), you make some new calls? I'd expect any new calls to to rejected or something, but would they slow down the shutting down process?
There was a problem hiding this comment.
Yeah, I did one call after the server.stopped returned to ensure that the server is closed and doesn't accept new connections/calls
There was a problem hiding this comment.
Ah but what if new calls come in after you call handle.stop() but before handle.stopped().await; finishes?
Eg if the server is quite busy and new connections keep coming in; will it still shutdown?
There was a problem hiding this comment.
it's not tested here indeed but I think it should be possible to add
There was a problem hiding this comment.
Sounds good to me offhand! I assume that without your fix above this fails?
Yes, it fails without my fix
There was a problem hiding this comment.
it's not tested here indeed but I think it should be possible to add
It'd be good to check this somehow I think just to make sure that continuing new connections won't stop the server from ever shutting down :)
Yes, it fails without my fix
Woop!
…nto na-fix-http-graceful-shutdown
Close #1089