If a server sends a stream error of type "protocol error" to a client,
that's the server saying "you're speaking http2 wrong". At that point,
regardless of whether we're in the right or not (that is, regardless of
whether the Transport is bug-free), clearly there's some confusion and
one of the two parties is either wrong or confused. There's no point
pushing on and trying to use the connection and potentially exacerbating
the confusion (as we saw in golang/go#47635).
Instead, make the client "poison" the connection by setting a new "do
not reuse" bit on it. Existing streams can finish up but new requests
won't pick that connection.
Also, make those requests as retryable without the caller getting an
error.
Given that golang/go#42777 existed, there are HTTP/2 servers in the
wild that incorrectly set RST_STREAM PROTOCOL_ERROR codes. But even
once those go away, this is still a reasonable fix for preventing
a broken connection from being stuck in the connection pool that fails
all future requests if a similar bug happens in another HTTP/2 server.
Updates golang/go#47635
Change-Id: I3f89ecd1d3710e49f7219ccb846e016eb269515b
Reviewed-on: https://go-review.googlesource.com/c/net/+/347033
Trust: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
It apparently used to be called connErrorReason but when it was
renamed the comment wasn't updated. Also, the comment was split via a
blank line, detaching it from its decl node.
Change-Id: I4ae510fc0e48fd61f40489428f9da4c6cab3ef2f
Reviewed-on: https://go-review.googlesource.com/45273
Reviewed-by: Hiroshi Ioka <hirochachacha@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
If the Transport got a stream error on the response headers, it was
never unblocking the client. Previously, Response.Body reads would be
aborted with the stream error, but RoundTrip itself would never
unblock.
The Transport now also sends a RST_STREAM to the server when we
encounter a stream error.
Also, add a "Cause" field to StreamError with additional detail. The
old code was just returning the detail, without the stream error
header.
Fixesgolang/go#16572
Change-Id: Ibecedb5779f17bf98c32787b68eb8a9b850833b3
Reviewed-on: https://go-review.googlesource.com/25402
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Andrew Gerrand <adg@golang.org>
HEADERS and CONTINUATION frames are special in that they must appear
contiguous on the wire and there are lots of annoying details to
verify while working through its state machine, including the handling
of hpack header list size limits and DoS vectors.
We now have three implementations of this merging (Server, Transport,
and grpc), and grpc's is not complete. The Transport's was also
partially incomplete.
Move this up to the Framer (opt-in, for compatibility) and remove the
support from the Server and Transport. I can fix grpc later to use
this.
Recommended reviewing order:
* hpack.go exports the HeaderField.Size method and adds an IsPseudo
method.
* errors.go adds some new unexported error types, for testing.
* frame.go adds the new type MetaHeadersFrame.
* frame.go adds new fields on Framer for controlling how ReadFrame
behaves
* frame.go Framer.ReadFrame now calls the new Framer.readMetaFrame
method
* frame_test.go adds a bunch of tests. these are largely redundant
with the existing tests which were in server and transport
before. They really belong with frame_test.go, but I also don't want
to delete tests in a CL like this. I probably won't remove them
later either.
* server.go and transport.go can be reviewed in either order at this
point. Both are the fun part of this change: deleting lots of hairy
state machine code (which was redundant in at least 6 ways: server
headers, server trailers, client headers, client trailers, grpc
headers, grpc trailers...). Both server and transport.go have the
general following form:
- set Framer.ReadMetaHeaders
- stop handling *HeadersFrame and *ContinuationFrame; handle
*MetaHeadersFrame instead.
- delete all the state machine + hpack parsing callback hell
The diffstat numbers look like a wash once you exclude the new tests,
but this pays for itself by far when you consider the grpc savings as
well, and the increased simplicity.
Change-Id: If348cf585165b528b7d3ab2e5f86b49a03fbb0d2
Reviewed-on: https://go-review.googlesource.com/19726
Reviewed-by: Blake Mizerany <blake.mizerany@gmail.com>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Removes state machine complication and duplication out of Server &
Transport and puts it into the Framer instead (where it's nicely
tested).
Also, for testing, start tracking the reason for errors. Later we'll
use it in GOAWAY frames' debug data too.
Change-Id: Ic933654a33edb62b4432c28fe09f7bfdb6f9b334
Reviewed-on: https://go-review.googlesource.com/18101
Reviewed-by: Blake Mizerany <blake.mizerany@gmail.com>
