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
Implement graceful shutdown for http3 servers. #4407
base: master
Are you sure you want to change the base?
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.
Small typo
@marten-seemann Do you have time to take a look at this draft? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4407 +/- ##
==========================================
- Coverage 85.15% 84.59% -0.56%
==========================================
Files 154 154
Lines 14794 14990 +196
==========================================
+ Hits 12597 12680 +83
- Misses 1690 1791 +101
- Partials 507 519 +12 ☔ View full report in Codecov by Sentry. |
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.
Thank you for working on this @WeidiDeng!
I’ve been working on a big refactor of the http3 package (#4115, #4116, more coming), which at the end will enable HTTP datagram support, which will all be included in the next release. For proper datagram support, we’ll introduce some kind of connection and stream tracking (no PR yet), which would be helpful for this PR. I suggest we defer graceful shutdown until these PRs have landed.
http3/client.go
Outdated
config: conf, | ||
opts: opts, | ||
dialer: dialer, | ||
receivedGoawayID: quic.StreamID(-4), |
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 seems hacky. Introduce a protocol.InvalidStreamID instead?
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 got the stream ID from rfc 9000. For different kinds of streams, the criteria for invalid id is different. I think a function that verifies ID would be better instead of a new type.
@@ -217,7 +218,8 @@ type Server struct { | |||
mutex sync.RWMutex | |||
listeners map[*QUICEarlyListener]listenerInfo | |||
|
|||
closed bool | |||
closed bool | |||
connections map[*quic.Connection]func() |
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.
Better: Use the tracing ID as a map key.
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.
But the methods on the quic.Connection
needs to be called. To properly stop server from accepting new streams at the grace period's end. Malicious client may attempt to open new streams even if goaway is received. Goaway itself doesn't close a connection.
# Conflicts: # http3/client.go # http3/client_test.go # http3/server.go # http3/server_test.go
# Conflicts: # http3/client.go # http3/client_test.go # http3/conn.go # http3/server.go # http3/server_test.go
# Conflicts: # http3/client_test.go # http3/server_test.go
Fix 6195 for caddy.