Closed mightyguava closed 3 years ago
CC @bradfitz @tombergan
Per §8.1.2.2:
HTTP/2 does not use the Connection header field to indicate connection-specific header fields; in this protocol, connection-specific metadata is conveyed by other means. An endpoint MUST NOT generate an HTTP/2 message containing connection-specific header fields; any message containing connection-specific header fields MUST be treated as malformed (Section 8.1.2.6).
I don't see anything in the x/net/http2
or x/net
documentation that suggests an exception to this rule.
I don't know whether there is a documented, supported way to provoke a Go HTTP/2 server to gracefully close the connection after serving a request, but Connection: close
does not appear to be it.
@bcmills, the http2 server already maps a Handler's Connection: close
response to a GOAWAY, as the OP referenced in their comment.
The x/net/http2/server.go
code is:
// "Connection" headers aren't allowed in HTTP/2 (RFC 7540, 8.1.2.2),
// but respect "Connection" == "close" to mean sending a GOAWAY and tearing
// down the TCP connection when idle, like we do for HTTP/1.
// TODO: remove more Connection-specific header fields here, in addition
// to "Connection".
if _, ok := rws.snapHeader["Connection"]; ok {
v := rws.snapHeader.Get("Connection")
delete(rws.snapHeader, "Connection")
if v == "close" {
rws.conn.startGracefulShutdown()
}
}
This looks like the server & client were both speaking at the ~same time and had messages that crossed paths in flight:
The server was sending:
GOAWAY
The client was sending:
HEADERS+DATA (a POST request with a body)
The server got the HEADERS and ignored it, since it was already inGoAway
state, and just assumed that the client might've already had that in flight.
func (sc *serverConn) processHeaders(f *MetaHeadersFrame) error {
sc.serveG.check()
id := f.StreamID
if sc.inGoAway {
// Ignore.
return nil
}
But it seems like that safety measure was only added for GET requests, because when we get the POST requests body in te DATA frame:
func (sc *serverConn) processData(f *DataFrame) error {
sc.serveG.check()
if sc.inGoAway && sc.goAwayCode != ErrCodeNo {
return nil
}
data := f.Data()
// "If a DATA frame is received whose stream is not in "open"
// or "half closed (local)" state, the recipient MUST respond
// with a stream error (Section 5.4.2) of type STREAM_CLOSED."
id := f.Header().StreamID
state, st := sc.state(id)
if id == 0 || state == stateIdle {
// Section 5.1: "Receiving any frame other than HEADERS
// or PRIORITY on a stream in this state MUST be
// treated as a connection error (Section 5.4.1) of
// type PROTOCOL_ERROR."
return ConnectionError(ErrCodeProtocol)
}
I don't remember why the sc.goAwayCode != ErrCodeNo
check is there. But that looks like the problem because our error code is ErrCodeNo
, and thus we're hitting the ErrCodeProtocol
at the bottom there.
Looks like it was added in https://github.com/golang/net/commit/569280fa63be4e201b975e5411e30a92178f0118
Not sure if it's related, but in the server handler, I also occasionally saw errors like request declared a Content-Length of 258 but only wrote 0 bytes
reading the request body
@bcmills @bradfitz any plans to fix this? I can take a stab at it too if you want to give me some pointers.
Change https://golang.org/cl/188360 mentions this issue: http2: discard DATA frames with higher stream IDs during graceful shutdown
@bradfitz @tombergan @bcmills this tiny CL has been open for almost a month now. It's mostly documentation updates with a tiny bug fix. Would one of you mind reviewing?
I've been on parental leave for 3 months. I'm just back at work and catching up.
@bradfitz got time to give a final stamp? Andrew already gave a +1. It's a super tiny change that I'd really love to close out. Should only take a moment of your time.
@mightyguava, it needs a test.
Change https://golang.org/cl/237957 mentions this issue: net/http2: discard DATA frames with higher stream IDs during graceful shutdown
Change https://golang.org/cl/296789 mentions this issue: all: update golang.org/x/* dependencies
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env
)?This reproduces on Linux as well. I've been grabbing debugging data from my mac though.
go env
OutputWhat did you do?
We are running http2 servers behind a TCP ELB loadbalancer (we need to terminate SSL at the server). This is causing unbalanced traffic since clients try to multiplex a single http2 connection. We had the idea of randomly sending GOAWAY to get clients to close their connections.
I then ran fake traffic using Vegeta to my server. So Go client/server.
What did you expect to see?
I expect that when a GOAWAY frame is sent, the server will complete processing of any open requests, and the client will open a new connection for new requests. This should all be transparent to user code. (I expected maybe I'd see
http.Client
return some errors about GOAWAY, but haven't seen that)What did you see instead?
The connection close chance was set at 0.5. At low qps, <10, everything is fine, no errors. Ramping up qps to 50, I start seeing
PROTOCOL_ERROR
coming from the server. This is reproducible locally with no load balancer in between client/server.Here's server and client logs for such occurrence: