code review 6238043: net/http: make client await response concurrently with ...

net/http: make client await response concurrently with writing request

If the server replies with an HTTP response before we're done
writing our body (for instance "401 Unauthorized" response), we
were previously ignoring that, since we returned our write
error ("broken pipe", etc) before ever reading the response.
Now we read and write at the same time.

Fixes issue 3595

[bradfitz, 2012-05-23 22:56:19]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg

[bradfitz, 2012-05-24 00:01:29]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2012-05-24 00:02:44]

PTAL (or a first look)

This CL now works, and is now updated with the server-side fix too.


On Wed, May 23, 2012 at 5:01 PM, <bradfitz@golang.org> wrote:

> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
http://codereview.appspot.com/**6238043/<http://codereview.appspot.com/6238043/>
>

[rsc, 2012-05-29 17:05:50]

I'm sad about how crufty the http code is getting. I realize that dealing with
real servers is full of corner cases, but I wonder if it's an indication that
the approach is not right.

http://codereview.appspot.com/6238043/diff/5002/src/pkg/net/http/server.go
File src/pkg/net/http/server.go (right):

http://codereview.appspot.com/6238043/diff/5002/src/pkg/net/http/server.go#ne...
src/pkg/net/http/server.go:580: time.Sleep(250 * time.Millisecond)
This needs a comment.

http://codereview.appspot.com/6238043/diff/5002/src/pkg/net/http/server.go#ne...
src/pkg/net/http/server.go:680: if w.requestBodyLimitHit {
Call c.finalFlush here?

http://codereview.appspot.com/6238043/diff/5002/src/pkg/net/http/transport.go
File src/pkg/net/http/transport.go (right):

http://codereview.appspot.com/6238043/diff/5002/src/pkg/net/http/transport.go...
src/pkg/net/http/transport.go:653: // Write the request concurrently with
waiting for a response,
It seems pretty heavy-weight to kick off a new goroutine to read every response,
just to work around broken servers.  Can we avoid this?

[bradfitz, 2012-05-29 17:11:13]

Any alternative proposals?

On Tue, May 29, 2012 at 10:05 AM, <rsc@golang.org> wrote:

> I'm sad about how crufty the http code is getting. I realize that
> dealing with real servers is full of corner cases, but I wonder if it's
> an indication that the approach is not right.
>
>
>
> http://codereview.appspot.com/**6238043/diff/5002/src/pkg/net/**
>
http/server.go<http://codereview.appspot.com/6238043/diff/5002/src/pkg/net/http/server.go>
> File src/pkg/net/http/server.go (right):
>
> http://codereview.appspot.com/**6238043/diff/5002/src/pkg/net/**
>
http/server.go#newcode580<http://codereview.appspot.com/6238043/diff/5002/src/pkg/net/http/server.go#newcode580>
> src/pkg/net/http/server.go:**580: time.Sleep(250 * time.Millisecond)
> This needs a comment.
>
> http://codereview.appspot.com/**6238043/diff/5002/src/pkg/net/**
>
http/server.go#newcode680<http://codereview.appspot.com/6238043/diff/5002/src/pkg/net/http/server.go#newcode680>
> src/pkg/net/http/server.go:**680: if w.requestBodyLimitHit {
> Call c.finalFlush here?
>
> http://codereview.appspot.com/**6238043/diff/5002/src/pkg/net/**
>
http/transport.go<http://codereview.appspot.com/6238043/diff/5002/src/pkg/net/http/transport.go>
> File src/pkg/net/http/transport.go (right):
>
> http://codereview.appspot.com/**6238043/diff/5002/src/pkg/net/**
>
http/transport.go#newcode653<http://codereview.appspot.com/6238043/diff/5002/src/pkg/net/http/transport.go#newcode653>
> src/pkg/net/http/transport.go:**653: // Write the request concurrently
>
> with waiting for a response,
> It seems pretty heavy-weight to kick off a new goroutine to read every
> response, just to work around broken servers.  Can we avoid this?
>
>
http://codereview.appspot.com/**6238043/<http://codereview.appspot.com/6238043/>
>

[rsc, 2012-05-29 17:13:12]

On Tue, May 29, 2012 at 1:11 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Any alternative proposals?

Does this case matter anymore, with the server changes?

Russ

[rsc, 2012-05-29 17:17:01]

On Tue, May 29, 2012 at 1:12 PM, Russ Cox <rsc@golang.org> wrote:
> Does this case matter anymore, with the server changes?

If so, one possibility would be to have a persistent goroutine
associated with the connection whose only job is to write things. You
could send the write requests there. That would at least be a little
nicer - especially more obviously serialized - than a new goroutine
each time.

Russ

[bradfitz, 2012-05-29 17:28:03]

On Tue, May 29, 2012 at 10:16 AM, Russ Cox <rsc@golang.org> wrote:

> On Tue, May 29, 2012 at 1:12 PM, Russ Cox <rsc@golang.org> wrote:
> > Does this case matter anymore, with the server changes?
>
> If so, one possibility would be to have a persistent goroutine
> associated with the connection whose only job is to write things. You
> could send the write requests there. That would at least be a little
> nicer - especially more obviously serialized - than a new goroutine
> each time.


Sure, that works.

The client change is required, even with the server part.

I only did the server part in this CL because it's related and I needed to
do it in order to test the client.  But I could break out the server part
separately and submit that first if desired?

[rsc, 2012-05-29 17:31:05]

Sure, splitting them sounds good.

[bradfitz, 2012-05-29 21:48:55]

Hello rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2012-06-03 23:37:59]

ping

[adg, 2012-06-18 15:39:42]

LGTM

http://codereview.appspot.com/6238043/diff/6005/src/pkg/net/http/transport.go
File src/pkg/net/http/transport.go (right):

http://codereview.appspot.com/6238043/diff/6005/src/pkg/net/http/transport.go...
src/pkg/net/http/transport.go:625: wr.ch <- errors.New("http: can't write HTTP
request on dead connection")
s/dead/broken/ ?

[rsc, 2012-06-19 04:13:38]

I'm not sure I really understand what's going on here anymore.

http://codereview.appspot.com/6238043/diff/6005/src/pkg/net/http/transport.go
File src/pkg/net/http/transport.go (right):

http://codereview.appspot.com/6238043/diff/6005/src/pkg/net/http/transport.go...
src/pkg/net/http/transport.go:642: func (re responseAndError) ok() bool { return
re.res != nil || re.err != nil }
This needs a comment. I would have expected that ok meant err == nil, for
example. What is being tested here?

[adg, 2012-06-19 04:38:40]

http://codereview.appspot.com/6238043/diff/6005/src/pkg/net/http/transport.go
File src/pkg/net/http/transport.go (right):

http://codereview.appspot.com/6238043/diff/6005/src/pkg/net/http/transport.go...
src/pkg/net/http/transport.go:642: func (re responseAndError) ok() bool { return
re.res != nil || re.err != nil }
On 2012/06/19 04:13:38, rsc wrote:
> This needs a comment. I would have expected that ok meant err == nil, for
> example. What is being tested here?

Perhaps a better name is 'valid'. Although it might be clearer to just hoist the
function out into the for loop in which it is used. (it's not used anywhere
else, is it?)

[rsc, 2012-06-19 16:02:44]

LGTM

I understand this better on a big screen. :-)

http://codereview.appspot.com/6238043/diff/6005/src/pkg/net/http/transport.go
File src/pkg/net/http/transport.go (right):

http://codereview.appspot.com/6238043/diff/6005/src/pkg/net/http/transport.go...
src/pkg/net/http/transport.go:642: func (re responseAndError) ok() bool { return
re.res != nil || re.err != nil }
On 2012/06/19 04:38:40, adg wrote:
> On 2012/06/19 04:13:38, rsc wrote:
> > This needs a comment. I would have expected that ok meant err == nil, for
> > example. What is being tested here?
> 
> Perhaps a better name is 'valid'. Although it might be clearer to just hoist
the
> function out into the for loop in which it is used. (it's not used anywhere
> else, is it?)

maybe done?

[bradfitz, 2012-06-19 16:04:50]

http://codereview.appspot.com/6238043/diff/6005/src/pkg/net/http/transport.go
File src/pkg/net/http/transport.go (right):

http://codereview.appspot.com/6238043/diff/6005/src/pkg/net/http/transport.go...
src/pkg/net/http/transport.go:642: func (re responseAndError) ok() bool { return
re.res != nil || re.err != nil }
On 2012/06/19 16:02:44, rsc wrote:
> On 2012/06/19 04:38:40, adg wrote:
> > On 2012/06/19 04:13:38, rsc wrote:
> > > This needs a comment. I would have expected that ok meant err == nil, for
> > > example. What is being tested here?
> > 
> > Perhaps a better name is 'valid'. Although it might be clearer to just hoist
> the
> > function out into the for loop in which it is used. (it's not used anywhere
> > else, is it?)
> 
> maybe done?

Or could invert it and named it like time.Time's IsZero().

[bradfitz, 2012-06-19 16:14:22]

Hello rsc@golang.org, adg@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2012-06-19 16:16:50]

Hello rsc@golang.org, adg@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2012-06-19 16:20:46]

*** Submitted as http://code.google.com/p/go/source/detail?r=305f67dc3f99 ***

net/http: make client await response concurrently with writing request

If the server replies with an HTTP response before we're done
writing our body (for instance "401 Unauthorized" response), we
were previously ignoring that, since we returned our write
error ("broken pipe", etc) before ever reading the response.
Now we read and write at the same time.

Fixes issue 3595

R=rsc, adg
CC=golang-dev
http://codereview.appspot.com/6238043