httplex was the original package name for shared code between net/http
and x/net/http2, but its name was too specific, and http/httpguts was
added later for other shared code.
We discussed merging httplex into httpguts at the time, but it didn't
happen earlier. This finishes the move.
Updates golang/go#23908
Change-Id: Ic7d6f39e584ca579d34b5ef5ec6a0c002a38a83c
Reviewed-on: https://go-review.googlesource.com/111875
Reviewed-by: Andrew Bonventre <andybons@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Introduce a common package x/net/http/httpguts which can be vendored by
net/http to share detail implementations of the HTTP specification with
x/net/http2.
Updates golang/go#23908
Change-Id: Id5a2d51e05135436cf406c4c4d1b13fca7f84a32
Reviewed-on: https://go-review.googlesource.com/104042
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
iana.org, www.iana.org and data.iana.org all present a valid TLS
certificate, so let's use it when fetching data to avoid errors in
transit.
Change-Id: I1f295442d24a221fe2b722c4782dceee38b960ec
Reviewed-on: https://go-review.googlesource.com/89415
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
In a given program there may be two separate copies of
ErrNoCachedConn: the h2_bundle.go version in net/http, and the user's
golang.org/x/net/http2 version. We need to be able to detect either
in net/http.
This CL adds a function to report whether an error value represents
that type of error, and then a subsequent CL to net/http will use it
instead of ==.
Updates golang/go#22091
Change-Id: I86f1e20704eee29b8980707b700d7a290107dfd4
Reviewed-on: https://go-review.googlesource.com/87297
Reviewed-by: Tom Bergan <tombergan@google.com>
That test makes a request with no body and receives a response with no
body. The client will receive a HEADERS frame with END_STREAM. The test
assumes that the stream is closed immediately on receipt of that HEADERS
frame, i.e., before RoundTrip returns.
This assumption was broken by https://golang.org/cl/70510, which made
stream closure asynchronous w.r.t. RoundTrip.
To fix TestCloseIdleConnections_h2 while preserving the intent of CL
70510, we break processHeaders into two cases:
1. The request has a body. In this case, END_STREAM puts the stream in a
half-closed-remote state, which means the connection is not
necessarily idle when RoundTrip returns (since the request body is
still being uploaded). In this case, we preserve the behavior from CL
70510.
2. The request does not have a body. In this case, END_STREAM puts the
stream in a closed state and we must close the stream before
returning from RoundTrip.
The following command passes when this CL is merged into net/http:
go test -count=100000 -run=TestCloseIdleConnections_h2 net/http
Updates golang/go#22413
Change-Id: Iff2a0685a636ad51bff380e86a42b0d0eea984e5
Reviewed-on: https://go-review.googlesource.com/80139
Run-TryBot: Tom Bergan <tombergan@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
In invalid response tests logger write error messages to stderr and spam
test output.
Since we know response are invalid in these tests we can safely discard
logger output.
Fixesgolang/go#22850
Change-Id: Id8c97be910f0cf7dbe2380ba632960364bc8478b
Reviewed-on: https://go-review.googlesource.com/80235
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
AFAICT, activeRes serves no real purpose. It is used in just two ways:
- To reduce the number of calls to closeIfIdle, which reduces the number
of acquires of cc.mu when there are many concurrent streams. I dug
through the CL history and could not find any benchmarks showing that
this is necessary.
- To avoid redundant calls to cs.bufPipe.CloseWithError(err) when a read
loop is shutdown. This is unnecessary, since redundant CloseWithError
calls are ignored.
Since there isn't a good reason to have activeRes, the simplest way to
fix the leak is to remove activeRes entirely.
Updates golang/go#21543
Change-Id: I1d1d2dc6c946425a2772c8bf71436707021ac269
Reviewed-on: https://go-review.googlesource.com/80137
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This fixes TestTrailersClientToServer_h2. Before this CL, the following
command reliably fails. With this CL merged into net/http, the following
command reliably succeeds.
go test -race -run=TestTrailersClientToServer_h2 -count 1000 net/http
Updates golang/go#22721
Change-Id: I05d1504c60854fcf3ae9531f36a126e94b00f0b7
Reviewed-on: https://go-review.googlesource.com/79238
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
In golang/go#22880, our http2 server was sending a HEADERS response
without a :status header. Our client code correctly returned an error
from RoundTrip, but we forgot to clean up properly, and then
subsequently crashed on a DATA frame.
This fixes the Transport crash. A fix for the server bug will come separately.
Change-Id: Iea3bcf4a8c95963c8b5e2b6dd722177607bd1bc1
Reviewed-on: https://go-review.googlesource.com/80056
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Tom Bergan <tombergan@google.com>
The HTTP/2 RFC does indeed mandate TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
but in practice, people are also using TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
becuase they are only using an ECDSA certificate. This is the case in acme/autocert.
It doesn't make sense to enforce only RSA in cipher suites if it will
never be used because they are using a ECDSA certificate.
Change-Id: I86dac192a3eb9b74e4268310a3b550b3bd88a37f
Reviewed-on: https://go-review.googlesource.com/30721
Reviewed-by: Tom Bergan <tombergan@google.com>
Run-TryBot: Tom Bergan <tombergan@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Based on golang/go#19653, it should be possible to reuse an http.Request
object after the outstanding request has completed. This CL fixes a race
in the http/2 library that occurs when a caller tries to reuse an
http.Request just after the request completed.
The new test failed with -race before this CL and passes after this CL.
Verified with -count 10000.
Updates golang/go#21316
Change-Id: I014cf9cefd0dd21f6f41763ba554d23ddc7fca40
Reviewed-on: https://go-review.googlesource.com/75530
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
There was a case where we forgot to undo this wrapper. Instead of fixing
that case, I moved the implementation of ClientConn.RoundTrip into an
unexported method that returns the same info as a bool.
Fixesgolang/go#22136
Change-Id: I7e5fc467f9c26fb74b9b83f2b3b7f8882645e34c
Reviewed-on: https://go-review.googlesource.com/75252
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Currently, we close the connection immediately after sending a GOAWAY
frame if all outstanding responses have been completely sent. However,
the client may have had requests in-flight at that time which have been
queued in the kernel receive buffer. On both Windows and Linux, if the
connection is close()'d when the receive buffer is not empty, the kernel
sends RST. This has the effect of aborting both sides of the connection,
meaning the client may not actually receive all responses that were sent
before the GOAWAY.
Instead, we should delay calling close() until after the receive buffer
has been drained. We don't want to delay indefinitely, which means we
need some kind of timeout. Ideally that timeout should be about 1 RTT +
epsilon, under the assumption that the client will not send any more
frames after receiving the GOAWAY. However, 1 RTT is difficult to
measure. It turns out we were already using a 1 second delay in other
cases, so we reuse that same delay here.
Note that we do not call CloseWrite() to half-close the underlying TLS
connection. This seems unnecessary -- GOAWAY is effectively a half-close
at the HTTP/2 level.
Updates golang/go#18701 (fixes after it's bundled into net/http)
Change-Id: I4d68bada6369ba95e5db02afe6dfad0a393c0334
Reviewed-on: https://go-review.googlesource.com/71372
Run-TryBot: Tom Bergan <tombergan@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
When a client receives a HEADER frame with a END_STREAM flag,
clientConn.streamByID closes the stream before processing the headers
which may contain a full non-error response. This causes the request's
bodyWriter cancelation to race with the response.
Closing the stream after processing headers allows the response to be
available before the bodyWriter is canceled.
Updates golang/go#20521
Change-Id: I70740e88f75240836e922163a54a6cd89535f7b3
Reviewed-on: https://go-review.googlesource.com/70510
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
Reviewed-by: Tom Bergan <tombergan@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Add a new peerMaxHeaderListSize member to ClientConn which records the
SETTINGS_MAX_HEADER_LIST_SIZE requested by the client's peer, and
respect this limit in (*ClientConn) encodeHeaders / encodeTrailers.
Attempting to send more than peerMaxHeaderListSize bytes of headers or
trailers will result in RoundTrip returning errRequestHeaderListSize.
Updates golang/go#13959
Change-Id: Ic707179782acdf8ae543429ea1af7f4f30e67e59
Reviewed-on: https://go-review.googlesource.com/29243
Run-TryBot: Tom Bergan <tombergan@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Tom Bergan <tombergan@google.com>
If the server returns a DATA frame before a HEADERS frame, the client
must close the connection with a PROTOCOL_ERROR as describe in the
section 8.1.2.6.
The current implementation does not reject such sequence and panics
trying to write on the non-initialized bufPipe.
Fixesgolang/go#21466
Change-Id: I55755dba259d026529a031e9ff82c915f5e4afae
Reviewed-on: https://go-review.googlesource.com/56770
Reviewed-by: Tom Bergan <tombergan@google.com>
Run-TryBot: Tom Bergan <tombergan@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Currently if the http2.Transport hits SettingsMaxConcurrentStreams for a
server, it just makes a new TCP connection and creates the stream on the
new connection. This CL updates that behavior to instead block RoundTrip
until a new stream is available.
I also fixed a second bug, which was necessary to make some tests pass:
Previously, a stream was removed from cc.streams only if either (a) we
received END_STREAM from the server, or (b) we received RST_STREAM from
the server. This CL removes a stream from cc.streams if the request was
cancelled (via ctx.Close, req.Cancel, or resp.Body.Close) before
receiving END_STREAM or RST_STREAM from the server.
Updates golang/go#13774
Updates golang/go#20985
Updates golang/go#21229
Change-Id: I660ffd724c4c513e0f1cc587b404bedb8aff80be
Reviewed-on: https://go-review.googlesource.com/53250
Run-TryBot: Tom Bergan <tombergan@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
We still support Go 1.6 for a few more days. (it'd normally be dropped
after Go 1.9 final is out)
And maybe we'll need to make a special case for supporting it longer
than normal if gRPC needs to.
Change-Id: I78675f1ef26aa09436a70d0f8aa3a0958768dd14
Reviewed-on: https://go-review.googlesource.com/53641
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
RoundTrip will retry a request if it receives REFUSED_STREAM. To guard
against servers that use REFUSED_STREAM to encourage rate limiting, or
servers that return REFUSED_STREAM deterministically for some requests,
we retry after an exponential backoff and we cap the number of retries.
The exponential backoff starts on the second retry, with a backoff
sequence of 1s, 2s, 4s, etc, with 10% random jitter.
The retry cap was set to 6, somewhat arbitrarily. Rationale: this is
what Firefox does.
Updates golang/go#20985
Change-Id: I4dcac4392ac4a3220d6d839f28bf943fe6b3fea7
Reviewed-on: https://go-review.googlesource.com/50471
Run-TryBot: Tom Bergan <tombergan@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
If the transport had previously sent a RST_STREAM but had not yet
deleted the stream from its list of active streams, we should refund
connection-level flow control for any DATA frame received as such
DATA frames will never be read.
We already refund connection-level flow control if a stream closes with
unread data in bufPipe. However, when we receive a DATA frame after
reset, we don't bother writing it to bufPipe, so we have to refund the
flow control separately.
Updates golang/go#20469
Change-Id: I5a9810a5d6b1bd7e291173af53646246545a6665
Reviewed-on: https://go-review.googlesource.com/46591
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
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>
Case happens if Read is called after it has already returned an error
previously. Verified that the new TestPipeCloseWithError test fails
before this change but passes after.
Updates golang/go#20501
Change-Id: I636fbb194f2d0019b0722556cc25a88da2d18e13
Reviewed-on: https://go-review.googlesource.com/44330
Run-TryBot: Tom Bergan <tombergan@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
This is a better fix that https://golang.org/cl/43455. Instead of
creating a separate goroutine to wait for the global shutdown channel,
we reuse the new serverMsgCh, which was added in a prior CL.
We also use the new net/http.Server.RegisterOnShutdown method to
register a shutdown callback for each http2.Server.
Updates golang/go#20302
Updates golang/go#18471
Change-Id: Icf29d5e4f65b3779d1fb4ea92924e4fb6bdadb2a
Reviewed-on: https://go-review.googlesource.com/43230
Run-TryBot: Tom Bergan <tombergan@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
After a response body is closed, we keep writing to the bufPipe. This
accumulates bytes that will never be read, wasting memory. The fix is to
discard the buffer on pipe.BreakWithError.
Updates golang/go#20448
Change-Id: Ia2cf46cb8c401fd8091ef3785eb48fe7b188bb57
Reviewed-on: https://go-review.googlesource.com/43810
Run-TryBot: Tom Bergan <tombergan@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Move it to a _test.go file instead.
After CL 43190, the pair function is no longer used by the hpack
package, so it can be removed from it. It's still used by tests.
This can help avoid future readers spending time on figuring out
why pair exists in package, but is not used. It can also reduce
chance of the pair function unintentionally being used in the
package (if that needs to happen explicitly, then pair can be
easily moved back to package).
Change-Id: I12ee9b46f57a522bd09e5d0ee22296df4dee1f35
Reviewed-on: https://go-review.googlesource.com/43851
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
