Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(139)

Issue 6964043: code review 6964043: net/http: buffer before chunking (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 9 months ago by bradfitz
Modified:
9 years, 8 months ago
Reviewers:
albert.strasheim
CC:
golang-dev, dfc, minux1, rsc, adg, sanjay.m
Visibility:
Public.

Description

net/http: buffer before chunking This introduces a buffer between writing from a handler and writing chunks. Further, it delays writing the header until the first full chunk is ready. In the case where the first full chunk is also the final chunk (for small responses), that means we can also compute a Content-Length, which is a nice side effect for certain benchmarks. Fixes issue 2357

Patch Set 1 #

Patch Set 2 : diff -r 0f491e663a44 https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r 0f491e663a44 https://go.googlecode.com/hg/ #

Total comments: 2

Patch Set 4 : diff -r 0f491e663a44 https://go.googlecode.com/hg/ #

Total comments: 2

Patch Set 5 : diff -r 74e2affcfe39 https://go.googlecode.com/hg/ #

Total comments: 7

Patch Set 6 : diff -r fe640aeda5f2 https://go.googlecode.com/hg/ #

Patch Set 7 : diff -r ec1e87068221 https://go.googlecode.com/hg/ #

Patch Set 8 : diff -r ec1e87068221 https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+250 lines, -172 lines) Patch
M src/pkg/net/http/header.go View 1 2 3 4 1 chunk +10 lines, -0 lines 0 comments Download
M src/pkg/net/http/serve_test.go View 1 2 3 4 4 chunks +8 lines, -8 lines 0 comments Download
M src/pkg/net/http/server.go View 1 2 3 4 5 6 7 15 chunks +232 lines, -164 lines 0 comments Download

Messages

Total messages: 31
bradfitz
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
9 years, 9 months ago (2012-12-19 02:33:32 UTC) #1
dfc
> the first full chunk is ready. In the case where the first > full ...
9 years, 9 months ago (2012-12-19 02:35:37 UTC) #2
minux1
On 2012/12/19 02:35:37, brad wrote: > the first full chunk is ready. In the case ...
9 years, 9 months ago (2012-12-19 17:53:28 UTC) #3
bradfitz
On Wed, Dec 19, 2012 at 9:53 AM, <minux.ma@gmail.com> wrote: > On 2012/12/19 02:35:37, brad ...
9 years, 9 months ago (2012-12-19 18:29:35 UTC) #4
minux1
On Thu, Dec 20, 2012 at 2:29 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > The goal of ...
9 years, 9 months ago (2012-12-19 18:34:20 UTC) #5
bradfitz
On Wed, Dec 19, 2012 at 10:33 AM, minux <minux.ma@gmail.com> wrote: > > On Thu, ...
9 years, 9 months ago (2012-12-19 18:52:55 UTC) #6
bradfitz
Ping. R=rsc + anybody else On Tue, Dec 18, 2012 at 6:33 PM, <bradfitz@golang.org> wrote: ...
9 years, 8 months ago (2013-01-07 19:34:49 UTC) #7
adg
I have read through this a few times and I'm not sure I understand it ...
9 years, 8 months ago (2013-01-08 05:15:08 UTC) #8
bradfitz
https://codereview.appspot.com/6964043/diff/5001/src/pkg/net/http/server.go File src/pkg/net/http/server.go (right): https://codereview.appspot.com/6964043/diff/5001/src/pkg/net/http/server.go#newcode358 src/pkg/net/http/server.go:358: if w.req.Method == "HEAD" || code == StatusNotModified { ...
9 years, 8 months ago (2013-01-08 05:33:34 UTC) #9
adg
On 7 January 2013 21:33, <bradfitz@golang.org> wrote: > This is just code movement. I could ...
9 years, 8 months ago (2013-01-08 05:38:58 UTC) #10
bradfitz
https://codereview.appspot.com/6964043/diff/8001/src/pkg/net/http/server.go File src/pkg/net/http/server.go (right): https://codereview.appspot.com/6964043/diff/8001/src/pkg/net/http/server.go#newcode195 src/pkg/net/http/server.go:195: func cloneHeader(h Header) Header { On 2013/01/08 05:15:08, adg ...
9 years, 8 months ago (2013-01-08 19:23:15 UTC) #11
bradfitz
Hello golang-dev@googlegroups.com, dave@cheney.net, minux.ma@gmail.com, rsc@golang.org, adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
9 years, 8 months ago (2013-01-08 19:23:21 UTC) #12
bradfitz
Code is now rearranged so the diff is more readable. I can rearrange it back ...
9 years, 8 months ago (2013-01-08 19:24:13 UTC) #13
adg
LGTM All these wrapped writers are hard to keep straight in my head, but now ...
9 years, 8 months ago (2013-01-09 00:10:19 UTC) #14
sanjay.m
drive-by typo... https://codereview.appspot.com/6964043/diff/17001/src/pkg/net/http/server.go File src/pkg/net/http/server.go (right): https://codereview.appspot.com/6964043/diff/17001/src/pkg/net/http/server.go#newcode283 src/pkg/net/http/server.go:283: handlerDone bool // set true when the ...
9 years, 8 months ago (2013-01-09 02:51:47 UTC) #15
bradfitz
Hello golang-dev@googlegroups.com, dave@cheney.net, minux.ma@gmail.com, rsc@golang.org, adg@golang.org, balasanjay@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
9 years, 8 months ago (2013-01-10 00:46:41 UTC) #16
bradfitz
All done, and lots of comments and TODOs added. PTAL. On Wed, Jan 9, 2013 ...
9 years, 8 months ago (2013-01-10 00:49:01 UTC) #17
bradfitz
Andrew, What about the name "chunkWriter" instead of "toConnWriter"? It is the writer that writes ...
9 years, 8 months ago (2013-01-11 01:51:57 UTC) #18
adg
On 11 January 2013 12:51, Brad Fitzpatrick <bradfitz@golang.org> wrote: > What about the name "chunkWriter" ...
9 years, 8 months ago (2013-01-11 02:55:45 UTC) #19
bradfitz
On Thu, Jan 10, 2013 at 6:55 PM, Andrew Gerrand <adg@golang.org> wrote: > > On ...
9 years, 8 months ago (2013-01-11 18:03:09 UTC) #20
bradfitz
*** Submitted as https://code.google.com/p/go/source/detail?r=fc9545bf5a1f *** net/http: buffer before chunking This introduces a buffer between writing ...
9 years, 8 months ago (2013-01-11 18:03:46 UTC) #21
albert.strasheim
Howdy There seems to be something wrong according to the torture box: FAIL: TestServerBufferedChunking-10 at ...
9 years, 8 months ago (2013-01-14 08:30:10 UTC) #22
bradfitz
This CL changed nothing about concurrency. It did, however, change chunking. But the tests were ...
9 years, 8 months ago (2013-01-15 01:02:45 UTC) #23
albert.strasheim
Hello On Tue, Jan 15, 2013 at 3:02 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > This ...
9 years, 8 months ago (2013-01-15 09:16:59 UTC) #24
dfc
I know it is a big ask, but it would be a very useful data ...
9 years, 8 months ago (2013-01-15 09:21:08 UTC) #25
albert.strasheim
Hello On Tue, Jan 15, 2013 at 11:21 AM, Dave Cheney <dave@cheney.net> wrote: > I ...
9 years, 8 months ago (2013-01-15 09:36:41 UTC) #26
dfc
Thanks, that would be a very useful datapoint. On Tue, Jan 15, 2013 at 8:36 ...
9 years, 8 months ago (2013-01-15 09:42:14 UTC) #27
bradfitz
Reproduced on my laptop with GOMAXPROCS=4. Fix is https://codereview.appspot.com/7109043 On Tue, Jan 15, 2013 at ...
9 years, 8 months ago (2013-01-15 15:06:56 UTC) #28
bradfitz
Btw, we really should have at least one GOMAXPROCS >1 builder on the dashboard. But ...
9 years, 8 months ago (2013-01-15 15:09:09 UTC) #29
dfc
I set my Darwin builders to GOMAXPROCS=4 yesterday. On 16/01/2013, at 2:09, Brad Fitzpatrick <bradfitz@golang.org> ...
9 years, 8 months ago (2013-01-15 19:19:49 UTC) #30
dfc
9 years, 8 months ago (2013-01-15 20:03:52 UTC) #31
Great catch Brad!

Albert, thank you for being so persistent with this issue. I'm sorry
that I thought this was some weird Fedora kernel issue. At least for
every failure we find and fix we gain more confidence that the next
failure reported _is_ real.

On Wed, Jan 16, 2013 at 2:06 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Reproduced on my laptop with GOMAXPROCS=4.
>
> Fix is https://codereview.appspot.com/7109043
>
>
> On Tue, Jan 15, 2013 at 1:16 AM, Albert Strasheim <fullung@gmail.com> wrote:
>>
>> Hello
>>
>> On Tue, Jan 15, 2013 at 3:02 AM, Brad Fitzpatrick <bradfitz@golang.org>
>> wrote:
>> > This CL changed nothing about concurrency.
>> > It did, however, change chunking.  But the tests were adjusted.
>> > Is this a sporadic failure?  How to reproduce?  I have machines with
>> > over 10
>> > cores.
>>
>> Xeon E5-2670 0 @ 2.60GHz, performance governor, F16 64-bit kernel 3.3.8.
>>
>> Test always passes with GOMAXPROCS=1 and fails about 80% of the time
>> with GOMAXPROCS=2 or greater.
>>
>> Here's a few runs:
>>
>> $ GOMAXPROCS=1 ./http.test -test.v -test.run=TestServerBufferedChunking
>> === RUN TestServerBufferedChunking
>> --- PASS: TestServerBufferedChunking (0.00 seconds)
>> PASS
>> $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking
>> === RUN TestServerBufferedChunking-2
>> --- FAIL: TestServerBufferedChunking-2 (0.00 seconds)
>> serve_test.go:1165:     response didn't end with a single 3 byte 'xyz'
>> chunk; got:
>>         "HTTP/1.1 200 OK\r\nContent-Type: text/plain;
>> charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:27
>> GMT\r\nTransfer-Encoding: chunked\r\n\r\n"
>> FAIL
>> $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking
>> === RUN TestServerBufferedChunking-2
>> --- PASS: TestServerBufferedChunking-2 (0.00 seconds)
>> PASS
>> $ GOMAXPROCS=1 ./http.test -test.v -test.run=TestServerBufferedChunking
>> === RUN TestServerBufferedChunking
>> --- PASS: TestServerBufferedChunking (0.00 seconds)
>> PASS
>> $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking
>> === RUN TestServerBufferedChunking-2
>> --- PASS: TestServerBufferedChunking-2 (0.00 seconds)
>> PASS
>> $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking
>> === RUN TestServerBufferedChunking-2
>> --- FAIL: TestServerBufferedChunking-2 (0.00 seconds)
>> serve_test.go:1165:     response didn't end with a single 3 byte 'xyz'
>> chunk; got:
>>         "HTTP/1.1 200 OK\r\nContent-Type: text/plain;
>> charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:40
>> GMT\r\nTransfer-Encoding: chunked\r\n\r\n"
>> FAIL
>> $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking
>> === RUN TestServerBufferedChunking-2
>> --- FAIL: TestServerBufferedChunking-2 (0.00 seconds)
>> serve_test.go:1165:     response didn't end with a single 3 byte 'xyz'
>> chunk; got:
>>         "HTTP/1.1 200 OK\r\nContent-Type: text/plain;
>> charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:41
>> GMT\r\nTransfer-Encoding: chunked\r\n\r\n"
>> FAIL
>> $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking
>> === RUN TestServerBufferedChunking-2
>> --- PASS: TestServerBufferedChunking-2 (0.00 seconds)
>> PASS
>> $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking
>> === RUN TestServerBufferedChunking-2
>> --- PASS: TestServerBufferedChunking-2 (0.00 seconds)
>> PASS
>> $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking
>> === RUN TestServerBufferedChunking-2
>> --- FAIL: TestServerBufferedChunking-2 (0.00 seconds)
>> serve_test.go:1165:     response didn't end with a single 3 byte 'xyz'
>> chunk; got:
>>         "HTTP/1.1 200 OK\r\nContent-Type: text/plain;
>> charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:43
>> GMT\r\nTransfer-Encoding: chunked\r\n\r\n"
>> FAIL
>> $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking
>> === RUN TestServerBufferedChunking-2
>> --- FAIL: TestServerBufferedChunking-2 (0.00 seconds)
>> serve_test.go:1165:     response didn't end with a single 3 byte 'xyz'
>> chunk; got:
>>         "HTTP/1.1 200 OK\r\nContent-Type: text/plain;
>> charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:43
>> GMT\r\nTransfer-Encoding: chunked\r\n\r\n"
>> FAIL
>> $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking
>> === RUN TestServerBufferedChunking-2
>> --- FAIL: TestServerBufferedChunking-2 (0.00 seconds)
>> serve_test.go:1165:     response didn't end with a single 3 byte 'xyz'
>> chunk; got:
>>         "HTTP/1.1 200 OK\r\nContent-Type: text/plain;
>> charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:43
>> GMT\r\nTransfer-Encoding: chunked\r\n\r\n"
>> FAIL
>> $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking
>> === RUN TestServerBufferedChunking-2
>> --- PASS: TestServerBufferedChunking-2 (0.00 seconds)
>> PASS
>>
>> Regards
>>
>> Albert
>
>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b