Adapation of Blake's proposed fix in https://golang.org/cl/16463
with a few changes:
-- bug fix (advance the buffer after writing)
-- don't reacquire/release the buffer in the loop. it was done like
that in case the max frame size changed while writing. Instead, push
that down into awaitFlowControl since it has to acquire that lock
anyway. Now it returns between 1 and the lower of how much we read
and how much we're allowed to write.
This does mean that if we start a request with a max frame size of
32KB, we'll never write larger than 32KB frames until the the next
request (because our scratch buffer we read into is only 32KB), but we
will start writing smaller DATA frames immediately once we see the
peer's SETTINGS frame.
Change-Id: I47fc503062f9602fe448cf7a36fc500e5d6b8ef9
Reviewed-on: https://go-review.googlesource.com/16443
Reviewed-by: Blake Mizerany <blake.mizerany@gmail.com>
This commit fixes two bugs.
The first bug returned io.EOF when a zero bytes were read from the
request body.
The second bug was a hang where the Transport waited for more flow
tokens than initialWindowSize BEFORE sending the first data frame which
never gave the server a chance to send flow tokens, so the client never
got enough to unblock awaitFlowControl. This commit changes
awaitFlowControl to wait for for [1,max] tokens, where max is the length
of the scratch buffer.
Change-Id: Ibbac0a38cd672535917a38330998d3b48d46f5f1
Reviewed-on: https://go-review.googlesource.com/16411
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
This commit allows a client of Transport to supply their own Dial
function that assumes all TLS checks have been performed and the
returned net.Conn is an h2 ready client connection.
Change-Id: If35b5c47c3bd6912a990d6cd89feefa3303bb42b
Reviewed-on: https://go-review.googlesource.com/16289
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Noticed during a crash in earlier testing. This now matches all the
other channel sends, selecting on the serverConn's terminiation channel.
Change-Id: I27ad3f9fd2d61154a5b95da82735d34fc2cbbc0b
Reviewed-on: https://go-review.googlesource.com/16381
Reviewed-by: Blake Mizerany <blake.mizerany@gmail.com>
Some polish and testing remains, but this should be the bulk of the
Transport work that was remaining.
There's one notable (but easy) TODO about sending WINDOW_UPDATE frames
every few hundred MB or so, but this is a checkpoint.
Updates golang/go#6891
Change-Id: Iced9850804bf2c069c75118895ee7c3750ba31b5
Reviewed-on: https://go-review.googlesource.com/16310
Reviewed-by: Blake Mizerany <blake.mizerany@gmail.com>
Make the pipe code take an interface as the backing store. Now a pipe
is something that's goroutine-safe and does the Cond waits but its underlying data
is now an interface: anything that's a ReaderWriter with a Len method (such as a
*bytes.Buffer), or a fixedBuffer (renamed in this CL from 'buffer').
This opens the ground to having a non-fixed buffer used with pipe.
This also moves the CloseWithError code up into the pipe code, out of
fixedBuffer.
Change-Id: Ia3b853e8aa8920807b705ff4e41bed934a8c67b7
Reviewed-on: https://go-review.googlesource.com/16312
Reviewed-by: Blake Mizerany <blake.mizerany@gmail.com>
We decided to glue together the HTTP/1 and HTTP/2 Transports in the
other direction, having the HTTP/1 code (net/http.Transport) start the
flow.
Remove the http2 Transport.Fallback for now, rather than leaving it
half implemented.
For background, see https://golang.org/cl/16090
Updates golang/go#6891
Change-Id: I511bc6d35a1a9a8e20010bd95ff694a894f42aa4
Reviewed-on: https://go-review.googlesource.com/16181
Reviewed-by: Andrew Gerrand <adg@golang.org>
This adds the http2 Transport method needed by
https://golang.org/cl/16090 to glue the HTTP/1 Transport
(net/http.Transport) to the http2.Transport without throwing away
fresh TCP connections after the TLS handshake determines which
NPN/ALPN protocol was selected.
Updates golang/go#6891
Change-Id: Iba004c8b1a149a5ee6c755d9a3fc1b19856a4e47
Reviewed-on: https://go-review.googlesource.com/16180
Reviewed-by: Andrew Gerrand <adg@golang.org>
This changes makes sure we never write to *writeData in the ServeHTTP
goroutine until the serve goroutine is done with it.
Also, it makes sure we don't transition the stream to the closed state
on the final DATA frame concurrently with the write.
To fix both, the writeFrameAsync goroutine no longer replies directly back
to the ServeHTTP goroutine with the write result. It's now passed to
the serve goroutine instead, which looks at the frameWriteMsg to
decide how to advance the state machine, then signals the ServeHTTP
goroutine with the result, and then advances the state machine.
Because advancing the state machine could transition it to closed,
which the ServeHTTP goroutine might also be selecting on, make the
ServeHTTP goroutine prefer its frameWriteMsg response channel for errors
over the stream closure in its select.
Various code simplifications and robustness in the process.
Tests now pass reliably even with high -count values, -race on/off,
etc. I've been unable to make h2load be unhappy now either.
Thanks to Tatsuhiro Tsujikawa (Github user @tatsuhiro-t) for the bug
report and debugging clues.
Fixesgolang/go#12998
Change-Id: I441c4c9ca928eaba89fd4728d213019606edd899
Reviewed-on: https://go-review.googlesource.com/16063
Reviewed-by: Andrew Gerrand <adg@golang.org>
ConfigureServer now returns an error if your config is wrong. It
doesn't attempt to fix it for you. Adjust this test accordingly.
Change-Id: Ie3de878ff58cef454de0ec9ab1a10459ca0ddd2d
Reviewed-on: https://go-review.googlesource.com/16061
Reviewed-by: Adam Langley <agl@golang.org>
This is required to work with the upcoming rewrite of the
httptest.Server's connection tracking in
https://go-review.googlesource.com/#/c/15151/
which (at least as of patchset 7) doesn't forcefully tear
down a StateNew connection during Server.Wait. That might
change.
In any case, this adds support for ConnState, which users would
expect regardless of protocol in a mixed HTTP/1 and HTTP/2
environment.
Change-Id: I124aafec29dda123a018935fa306f465ae99cd97
Reviewed-on: https://go-review.googlesource.com/15913
Reviewed-by: Andrew Gerrand <adg@golang.org>
In general, clean up and simplify the handling of frame writing from
handler goroutines. Always select on streams closing, and don't try
to pass around and re-use channels. It was too confusing. Instead,
reuse channels in a very local manner that's easy to reason about.
Thanks to Github user @pabbott0 (who has signed the Google CLA) for
the initial bug report and test cases.
Fixesbradfitz/http2#45
Change-Id: Ib72a87cb6e33a4bb118ae23d765ba594e9182ade
Reviewed-on: https://go-review.googlesource.com/15820
Reviewed-by: Andrew Gerrand <adg@golang.org>
There was a design problem earlier where the serve goroutine assumed
that the readFrame goroutine could return only connection-level
errors, but the readFrames goroutine (and the underlying Framer)
assumed it could return stream-level errors (type StreamError) and
have them handled as stream errors in the layers above. That's how it
should have been, and what this CL does.
Now readFrames returns both the Frame and error from ReadFrames
together as a pair, and an error isn't necessarily fatal to the
connection.
Fixesgolang/go#12733Fixesbradfitz/http2#53
Change-Id: If4406ceaa019886893d3c61e6bfce25ef74560d3
Reviewed-on: https://go-review.googlesource.com/15735
Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
In the first attempt to enforce the SETTINGS_MAX_HEADER_LIST_SIZE
(https://go-review.googlesource.com/15751), the enforcement happened
in the hpack decoder and the hpack decoder returned errors on Write
and Close if the limit was violated. This was incorrect because the
decoder is used over the life of the connection and all subsequent
requests and could therefore get out of sync.
Instead, this moves the counting of the limit up to the http2 package
in the serverConn type, and replaces the hpack counting mechanism with
a simple on/off switch. When SetEmitEnabled is set false, the header
field emit callbacks will be suppressed and the hpack Decoder will do
less work (less CPU and garbage) if possible, but will still return
nil from Write and Close on valid input, and will still stay in sync
it the stream.
The http2 Server then returns a 431 error if emits were disabled while
processing the HEADER or any CONTINUATION frames.
Fixesgolang/go#12843
Change-Id: I3b41aaefc6c6ee6218225f8dc62bba6ae5fe8f2d
Reviewed-on: https://go-review.googlesource.com/15733
Reviewed-by: Andrew Gerrand <adg@golang.org>
Minor performance improvement, since len(staticTable) is then a
constant at compile time.
Change-Id: Ie51ecc985aa3f40d50f0a7d1ab6ac91738f696d5
Reviewed-on: https://go-review.googlesource.com/15731
Reviewed-by: Dmitri Shuralyov <shurcool@gmail.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
There are already tests which cover the behavior. This is simply
moving where it happens. I'm not adding new tests to cover the very
bad behavior because once the rest of the bug is fixed, it won't be
possible to observe anyway. But even for smallish inputs, this is
faster.
Part of golang/go#12843
Change-Id: I7d69f2409e2adb4a01d101a915e09cf887b03f21
Reviewed-on: https://go-review.googlesource.com/15601
Reviewed-by: Andrew Gerrand <adg@golang.org>
StripPrefix in the webdav package strips prefixes from requests
(including the Destination headers) but cannot handle the paths
in the xml entities responses which are confusing some clients
(e.g. cadaver).
This patch replaces StripPrefix with Prefix in the Handler to
handle prefixes both in the requests and in the xml entities
responses.
Change-Id: I67062e30337b2ae422c82a2f927454f5a8a00e34
Reviewed-on: https://go-review.googlesource.com/13857
Reviewed-by: Nigel Tao <nigeltao@golang.org>
These expose the main HTML rendering functions of this package
for use with alternate HTTP muxes and on alternate paths.
Fixesgolang/go#12195.
Change-Id: I679583fd26116bc83ff551a5d2a1d73ffa1e01f0
Reviewed-on: https://go-review.googlesource.com/13825
Reviewed-by: Dave Day <djd@golang.org>
