-
-
Notifications
You must be signed in to change notification settings - Fork 276
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 Graceful Shutdown support #250
Conversation
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.
Looks like a solid PR.
I reviewed closely, but it definitely is a pretty involved PR, so hopefully I didn't miss anything 👍
I had some minor inline comments.
@@ -11,8 +11,18 @@ pub struct Ping { | |||
payload: Payload, | |||
} | |||
|
|||
// This was just 8 randomly generated bytes. We use something besides just | |||
// zeroes to distinguish this specific PING from any other. | |||
const SHUTDOWN_PAYLOAD: Payload = [0x0b, 0x7b, 0xa2, 0xf0, 0x8b, 0x9b, 0xfe, 0x54]; |
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.
Perhaps we could use [0x0b, 0x7b, 0xf0, 0xa2, 0x8b, 0x9b, 0x54, 0xfe]
...
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.
PS: This is not a serious comment.
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.
I just ran node -p "require('crypto').randomBytes(8)"
and copied over. No particular reason other than wanting it be different from [0u8; 8]
.
src/proto/go_away.rs
Outdated
} | ||
|
||
#[derive(Debug)] | ||
struct GoingAway { |
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.
This looks like the same struct as frame::GoAway
. What is the reason to duplicate it? Could the reason be included in a comment for posterity?
src/server.rs
Outdated
/// Must continue being polled to close connection. | ||
/// | ||
/// For graceful shutdowns, see [`graceful_shutdown`](Connection::graceful_shutdown). | ||
pub fn close_immediately(&mut self, reason: Reason) { |
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.
Since this function does not actually close immediately (you have to continue to poll), maybe this could be named abrupt_shutdown
as that matches the naming of graceful_shutdown
.
52922cf
to
c864a6b
Compare
note there's a legit test failure in CI:
|
If graceful shutdown is initiated, a GOAWAY of the max stream ID - 1 is sent, followed by a PING frame, to measure RTT. When the PING is ACKed, the connection sends a new GOAWAY with the proper last processed stream ID. From there, once all active streams have completely, the connection will finally close.
/me shakes fist at flaky test XD |
- Adds `wait_for` that takes another future to signal the mock should continue. - Adds `yield_once` to allow one chain of futures to yield to the other.
c864a6b
to
74a497c
Compare
If graceful shutdown is initiated, a GOAWAY of the max stream ID - 1 is
sent, followed by a PING frame, to measure RTT. When the PING is ACKed,
the connection sends a new GOAWAY with the proper last processed stream
ID. From there, once all active streams have completely, the connection
will finally close.
Closes #245