http3: immediately close all connections on Server.Close#4689
http3: immediately close all connections on Server.Close#4689marten-seemann merged 2 commits intomasterfrom
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4689 +/- ##
==========================================
- Coverage 84.77% 84.76% -0.01%
==========================================
Files 150 150
Lines 14922 14929 +7
==========================================
+ Hits 12650 12654 +4
- Misses 1743 1745 +2
- Partials 529 530 +1 ☔ View full report in Codecov by Sentry. |
| // ServeQUICConn serves a single QUIC connection. | ||
| func (s *Server) ServeQUICConn(conn quic.Connection) error { | ||
| return s.handleConn(conn) | ||
| return s.handleConn(context.Background(), conn) |
There was a problem hiding this comment.
I don't think this is necessary. It will cause the quic connection to be shutdown. Users should coordinate this with the server shutdown, or not call shutdown at all.
There was a problem hiding this comment.
It's a background context, so it's never canceled. This means that when users pass a connection to ServeQUICConn, closing the server will not the connection. It's the user's responsibility to close that connection.
|
This matches the behavior of HTTP/1.1: https://gist.github.com/marten-seemann/daaeb2949cdebd68d6c93381f6589c17. Closing the server kills the request. |
|
I tested your patch, but the response is successful. The test is inside the integration test. The OS is windows 10 amd64.
Any idea why? |
|
|
Because it needs #4689. |
|
I added some of your changes though.
The first line is the debug output |
|
I see what's going on here. But I fixed it already. |
|
@WeidiDeng Let's merge this PR, then you can rebase #4669. |
|
I actually removed the new context parameter. I feel like my change just overwrites yours. |
We can deal with this on your PR. This PR is correct in and of itself, and we need a very similar behavior for graceful shutdown (once the context expires). |
cc @WeidiDeng