Implement graceful shutdown for http3 servers. #4407

WeidiDeng · 2024-04-03T03:23:06Z

Fix 6195 for caddy.

…nnections

francislavoie

Small typo

http3/client.go

WeidiDeng · 2024-04-05T01:51:12Z

@marten-seemann Do you have time to take a look at this draft?

codecov · 2024-04-07T02:37:28Z

Codecov Report

Attention: Patch coverage is 43.13725% with 116 lines in your changes are missing coverage. Please review.

Project coverage is 84.59%. Comparing base (e2fbf3c) to head (312a3d0).

Files	Patch %	Lines
http3/client.go	35.71%	42 Missing and 3 partials ⚠️
http3/server.go	61.17%	32 Missing and 8 partials ⚠️
http3/frames.go	0.00%	28 Missing ⚠️
http3/conn.go	0.00%	2 Missing and 1 partial ⚠️

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.
📢 Have feedback on the report? Share it here.

marten-seemann

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.

marten-seemann · 2024-04-07T14:26:10Z

http3/client.go

+ config: conf,
+ opts: opts,
+ dialer: dialer,
+ receivedGoawayID: quic.StreamID(-4),


This seems hacky. Introduce a protocol.InvalidStreamID instead?

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.

http3/frames.go

marten-seemann · 2024-04-07T14:30:41Z

http3/server.go

@@ -217,7 +218,8 @@ type Server struct {
 mutex sync.RWMutex
 listeners map[*QUICEarlyListener]listenerInfo

- closed bool
+ closed bool
+ connections map[*quic.Connection]func()


Better: Use the tracing ID as a map key.

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.

Better: Use the tracing ID as a map key.

Update: the tracing ID has now been deprecated, so we shouldn't use that one.

# 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

WeidiDeng · 2024-08-07T03:38:57Z

@marten-seemann When do you think this patch is ready to be reviewed? I haven't updated it in a while, and several of those enhancements already landed in the latest release. If you think it's ok to add graceful shutdown, I'll update this patch against the latest release.

marten-seemann · 2024-08-07T04:05:42Z

Sorry, I dropped the ball on this PR. Let's get this into the next release! I'll review it later.

marten-seemann

Is there any way we can solve this without having a map of connections in the Server. That feels kind of heavy. We already have a few Go routines blocking on AcceptStream, maybe we can use one of those to do the work of sending the GOAWAY?

marten-seemann · 2024-08-08T07:45:03Z

http3/server.go

+ ID: lastID + 4,
+ }).Append(b[:0])
+ str.Write(b)
+ mutex.Unlock()


str.Write might block indefinitely. You shouldn't hold the mutex that long.

The map is there to make sure all connection is closed before CloseGracefully returns. It's kept in a map because this way server can begin to send goaway frame this way instead of relying on AcceptStream to accept a stream and then write a goaway.

The alternative is to create a goroutine that listens for when the server is closed. But that's even heavier weight than the current method of tracking with map. I guess that's why stdlib uses maps to do the same.

@marten-seemann What do you suggest we use to track the accepted quic connections? In stdlib, http/http2 server both track them using a map.

For http2, http2 connections have a single goroutine that handles all the write requests. so there is no lock when writing the goway.

marten-seemann · 2024-08-08T07:45:39Z

http3/server.go

@@ -217,7 +218,8 @@ type Server struct {
 mutex sync.RWMutex
 listeners map[*QUICEarlyListener]listenerInfo

- closed bool
+ closed bool
+ connections map[*quic.Connection]func()


Better: Use the tracing ID as a map key.

Update: the tracing ID has now been deprecated, so we shouldn't use that one.

marten-seemann · 2024-08-08T07:49:12Z

http3/server.go

+ mutex sync.Mutex
+ closed bool
+ // client opened bidirectional stream ID starts at 0 and increase by 4
+ lastID = quic.StreamID(-4)


Suggested change

lastID = quic.StreamID(-4)

lastID = protocol.InvalidStreamID

Is it fine to not close quic.Connection on the server side. Right now it's only closed on the server side when an error occurred during request handling.

Why don't you want to close it? That's the point of graceful shutdown after all, isn't it?

Sending goaway frame requires the server having some kinds of references to the quic connection. Closing it is not necessary as stdlib doesn't close either.

@marten-seemann I'm thinking using a counter to track the number of accepted quic connections, and a context that will be canceled when shutdown is called. context. AfterFunc can be used to send goaway in this case. The server only knows about the number of the quic connections. What do you think of this method?

Also, perhaps I should implement server side graceful shutdown first, client side later. To keep changes small.

marten-seemann · 2024-08-08T07:49:55Z

http3/server.go

@@ -618,11 +695,54 @@ func (s *Server) Close() error {
 return err
 }

+func (s *Server) connectionsCount() int {


The function name suggests that this function doesn't have any side effects, but it clearly does.

marten-seemann · 2024-08-08T07:50:39Z

http3/server.go

+
+ var reject bool
+ mutex.Lock()
+ reject = closed


s.closed?

marten-seemann · 2024-08-08T07:53:31Z

http3/client.go

@@ -83,6 +88,9 @@ func (c *SingleDestinationRoundTripper) init() {
 c.decoder = qpack.NewDecoder(func(hf qpack.HeaderField) {})
 c.requestWriter = newRequestWriter()
 c.hconn = newConnection(c.Connection, c.EnableDatagrams, protocol.PerspectiveClient, c.Logger)
+ c.hconn.controlStrHandler = c.readControlStream
+ c.receivedGoawayID = quic.StreamID(-4)


Suggested change

c.receivedGoawayID = quic.StreamID(-4)

c.receivedGoawayID = protocol.InvalidStreamID

marten-seemann · 2024-08-08T07:55:21Z

http3/client.go

+ }
+
+ // Separate goroutine to prevent interference with request cancellation
+ go func() {


I might be missing something, but this looks to me like you're canceling requests that are in flight when receiving the GOAWAY. But that's exactly what we should not do: Requests still in flight should complete as if nothing happened.

marten-seemann · 2024-08-08T07:56:43Z

http3/frames.go

+const maxQuicVarintLen = 8
+
+func parseGoawayFrame(r io.Reader, l uint64) (*goawayFrame, error) {
+ if l > maxQuicVarintLen {


Easier: just parse the varint, and check that the length matches the number of bytes parsed afterwards.

There is a check afterwards to see if data read matches the varint it encodes. This check is necessary.

No, it is not. This code is just unnecessarily complicated. Use quicvarint.Read.

A malformed goaway frame can have an arbitrary large length defined. And quicvarint.Read is used later.

Just parse the two varints, then check that the length is consistent. Saves you 10 LOC and multiple allocs.

You have to wrap the reader with a bufio.Reader to be able to use Read varint. And to limit the number of bytes read a limited reader is needed, or else the stream may be corrupt due to more data read than needed.

Any feedbacks? @marten-seemann

My previous comment still stands.

Show me some code snippets. I don't think I know how to think of this. Double read?

Sure. I'll do this once the rest of the PR is in a shape where we could merge it.

http3/frames.go

# Conflicts: # http3/client.go # http3/conn.go # http3/server.go

update goaway frame parsing

WeidiDeng added 5 commits April 3, 2024 11:20

send goaway when server is shutdown and close all established quic co…

34653a9

…nnections

fix lint

a3926df

fix nil map and goaway frame writing

2bb230a

fix goaway handling

7fbf64a

fix request cancellation handling

b3e4dfc

WeidiDeng mentioned this pull request Apr 3, 2024

http: close http3 server gracefully caddyserver/caddy#6213

Draft

francislavoie reviewed Apr 3, 2024

View reviewed changes

http3/client.go Outdated Show resolved Hide resolved

WeidiDeng added 5 commits April 3, 2024 22:41

fix typo

8a62432

send goaway immediately when the server is closed

6d0eb78

embed control stream handler as a function

f909ab8

send correct error code

4b9ff1f

multiple goaway frames can't have larger stream id

ab78fa0

WeidiDeng added 4 commits April 5, 2024 13:33

fix goaway not handled in some cases

9076936

fix client side goaway handling

a5887ed

fix unit tests

5c88ecb

fix lint

d620dbe

WeidiDeng marked this pull request as ready for review April 7, 2024 02:30

marten-seemann reviewed Apr 7, 2024

View reviewed changes

WeidiDeng added 2 commits April 8, 2024 09:04

stricter goaway parsing and validation

5c93e35

set last received goaway id in control stream loop

6071e44

marten-seemann added the blocked label Apr 13, 2024

WeidiDeng added 7 commits April 26, 2024 14:09

Merge branch 'refs/heads/master' into graceful-close

70813b5

# Conflicts: # http3/client.go # http3/client_test.go # http3/server.go # http3/server_test.go

merge from upstream

e8ad09d

Merge branch 'refs/heads/master' into graceful-close

dbdcfde

# Conflicts: # http3/client.go # http3/client_test.go # http3/conn.go # http3/server.go # http3/server_test.go

merge from upstream

ece4a80

Merge branch 'refs/heads/master' into graceful-close

6173d4a

# Conflicts: # http3/client_test.go # http3/server_test.go

merge from upstream

e9e5c16

Merge branch 'refs/heads/master' into graceful-close

312a3d0

marten-seemann added this to the v0.47 milestone Aug 7, 2024

WeidiDeng marked this pull request as draft August 8, 2024 00:12

marten-seemann reviewed Aug 8, 2024

View reviewed changes

marten-seemann removed the blocked label Aug 9, 2024

WeidiDeng added 4 commits August 16, 2024 14:25

Merge branch 'master' into graceful-close

d54809f

# Conflicts: # http3/client.go # http3/conn.go # http3/server.go

merge from upstream;

8534efd

update goaway frame parsing

simplify goaway frame parsing

0414991

fix server race

8f5b6c2

marten-seemann modified the milestones: v0.47, v0.48 Sep 7, 2024

WeidiDeng mentioned this pull request Sep 11, 2024

http3: send GOAWAY when server is shutting down #4669

Open

	c.receivedGoawayID = quic.StreamID(-4)
	c.receivedGoawayID = protocol.InvalidStreamID

Implement graceful shutdown for http3 servers. #4407

Are you sure you want to change the base?

Implement graceful shutdown for http3 servers. #4407

Conversation

WeidiDeng commented Apr 3, 2024

francislavoie left a comment

Choose a reason for hiding this comment

WeidiDeng commented Apr 5, 2024 • edited Loading

codecov bot commented Apr 7, 2024 • edited Loading

Codecov Report

marten-seemann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WeidiDeng commented Aug 7, 2024

marten-seemann commented Aug 7, 2024

marten-seemann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WeidiDeng commented Apr 5, 2024 •

edited

Loading

codecov bot commented Apr 7, 2024 •

edited

Loading