code review 14438065: net/http: flush request body chunks to the wire.

net/http: flush request body chunks to the wire. Fixes issue 6574.

[benburkert, 2013-10-12 20:59:10]

Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go

[dsymonds, 2013-10-12 21:48:52]

This seems like it would reduce the performance of servers that don't care about your use case, and I think that would be most servers. 

[benburkert, 2013-10-12 22:14:06]

For server response writing chunks over 4096 (bufio.defaultBufSize) bytes, the extra overhead will be the check & cast to a *bufio.Writer, and the buf.Len() call. If that's too much overhead, then this could first test for small writes (if len(data) < 4096 { ... }) before the other checks. The drawback of this approach is that value to check against (4096 in this case) is arbitrary and the actual buffer size may be larger. Another approach would be to add a BufferSize field to http.Transport. Then use that value to with bufio.NewReaderSize & bufio.NewWriterSize here: http://golang.org/src/pkg/net/http/transport.go#L509 On Saturday, October 12, 2013 2:48:50 PM UTC-7, David Symonds wrote: > > This seems like it would reduce the performance of servers that don't > care about your use case, and I think that would be most servers. > 

[dsymonds, 2013-10-12 22:24:45]

I'm thinking about all the small writes. Your change is effectively removing the buffering entirely, unless I misunderstand it. 

[benburkert, 2013-10-12 22:46:28]

Yes, that is correct. But my guess is that there are not many servers that would be impacted by this. Relying on internal buffering to reduce the number and increase the size of TCP packets while writing multiple <4K chunks seems like an even more unlikely use case. On Saturday, October 12, 2013 3:24:44 PM UTC-7, David Symonds wrote: > > I'm thinking about all the small writes. Your change is effectively > removing the buffering entirely, unless I misunderstand it. > 

[minux1, 2013-10-12 22:53:34]

On Sat, Oct 12, 2013 at 6:46 PM, <ben@benburkert.com> wrote: > Yes, that is correct. But my guess is that there are not many servers that > would be impacted by this. Relying on internal buffering to reduce the > number and increase the size of TCP packets while writing multiple <4K > chunks seems like an even more unlikely use case. > i think we may as well just remove all the buffering (which means reverting https://codereview.appspot.com/6964043) in fact, i'm not even convinced the original issue is valid. you can always call Flush to force this behaviour. > On Saturday, October 12, 2013 3:24:44 PM UTC-7, David Symonds wrote: >> >> I'm thinking about all the small writes. Your change is effectively >> removing the buffering entirely, unless I misunderstand it. > > 

[benburkert, 2013-10-12 23:12:59]

Could you please expand upon this? As I understand it there is no way for the client to get ahold of the bufio.Writer held by the http.chunkedWriter. And a call to Flush does not propagate down the chain of io.Writer's, so calling Flush on the body from the client/server code would not help. I still think it's useful to support internal buffering. Perhaps the above mentioned change to Transport would be the best approach because it would not change the current behavior and allows for internal buffering to be disabled. On Saturday, October 12, 2013 3:53:14 PM UTC-7, minux wrote: > > in fact, i'm not even convinced the original issue is valid. you can > always call Flush > to force this behaviour. > 

[bradfitz, 2013-10-12 23:31:52]

I replied at https://code.google.com/p/go/issues/detail?id=6574 Let's discuss there, but ideally after Go 1.2 is out. On Sat, Oct 12, 2013 at 10:59 AM, <ben@benburkert.com> wrote: > Reviewers: golang-dev1, > > Message: > Hello golang-dev@googlegroups.com, > > I'd like you to review this change to > https://code.google.com/p/go > > > Description: > net/http: flush request body chunks to the wire. > Fixes issue 6574. > > Please review this at https://codereview.appspot.**com/14438065/<https://codereview.appspot.com/144... > > Affected files (+18, -0 lines): > M src/pkg/net/http/chunked.go > M src/pkg/net/http/chunked_test.**go > > > Index: src/pkg/net/http/chunked.go > ==============================**==============================**======= > --- a/src/pkg/net/http/chunked.go > +++ b/src/pkg/net/http/chunked.go > @@ -150,6 +150,11 @@ > if n, err = cw.Wire.Write(data); err != nil { > return > } > + if bw, ok := cw.Wire.(*bufio.Writer); ok { > + if bw.Buffered() > 0 { > + bw.Flush() > + } > + } > if n != len(data) { > err = io.ErrShortWrite > return > Index: src/pkg/net/http/chunked_test.**go > ==============================**==============================**======= > --- a/src/pkg/net/http/chunked_**test.go > +++ b/src/pkg/net/http/chunked_**test.go > @@ -8,6 +8,7 @@ > package http > > import ( > + "bufio" > "bytes" > "fmt" > "io" > @@ -75,6 +76,18 @@ > } > } > > +func TestChunkWriterFlush(t *testing.T) { > + buf := bytes.NewBuffer([]byte{}) > + > + cw := newChunkedWriter(bufio.**NewWriter(buf)) > + > + cw.Write([]byte("Test Chunk")) > + > + if buf.Len() == 0 { > + t.Errorf("written chunk buffered; not flushed to wire") > + } > +} > + > func TestParseHexUint(t *testing.T) { > for i := uint64(0); i <= 1234; i++ { > line := []byte(fmt.Sprintf("%x", i)) > > > -- > > ---You received this message because you are subscribed to the Google > Groups "golang-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou... > . > To view this discussion on the web visit https://groups.google.com/d/** > msgid/golang-dev/**20cf30223bdbafe1b004e8918145%**40google.com<https://groups.google.com/d/msgid/golang-dev/20cf30223bdbafe1b004e8918145%40google.com> > . > For more options, visit https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o... > . > 

[gobot, 2013-12-20 16:26:11]

Replacing golang-dev with golang-codereviews.

[gobot, 2013-12-20 16:26:11]

Replacing golang-dev with golang-codereviews.

[gobot, 2014-01-03 00:46:40]

R=bradfitz@golang.org (assigned by dsymonds@golang.org)

[bradfitz, 2014-01-03 17:44:39]

This isn't the right place to fix it. Doing it here means a normal io.Copy from something to this will result in lots of flushes. The people who want flushes here are rare (feeding an io.Pipe body slowly) and we should make this behavior explicit for those who want it. It should probably be a special type of request body or an option on Transport.

[rsc, 2014-03-05 20:01:01]

R=close