code review 4272045: http: export Transport, add keep-alive support

http: export Transport, add keep-alive support

This patch adds a connection cache and keep-alive
support to Transport, which is used by the
HTTP client.

It's also structured such that it's easy to add
HTTP pipelining in the future.

[bradfitz, 2011-03-12 00:55:10]

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

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

[bradfitz, 2011-03-14 22:57:38]

Russ, Petar,

I just realized I didn't use ClientConn for this.

Looking at ClientConn again (I always forget about it), I remembered why:
 it's not thread-safe.  ClientConn.Read says "Read can be
called concurrently with Write, but not with another Read.".

What do we want to do with ClientConn?  Once we have a good persistent
Transport (that's thread-safe and works with any host, as this CL does),
then ClientConn seems redundant.

Do we want to 1) ditch ClientConn or 2) enhance ClientConn such that it's
usable by Transport?  Or 3) keep ClientConn but not use it from Transport.

Personally I like Transport's communication with channels much more than all
the Lock/Unlock going on in ClientConn (good percent of the code).  And
because I don't know who would use ClientConn, I'm in favor of removing it
but maybe I'm not understanding its value.  Petar--- fill me in?

- Brad

On Fri, Mar 11, 2011 at 4:55 PM, <bradfitz@golang.org> wrote:

> Reviewers: rsc, petar-m,
>
> Message:
> Hello rsc, petar-m (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> http: export Transport, add keep-alive support
>
> ** WIP.  All tests pass, but I want more tests **
>
> This patch adds a connection cache and keep-alive
> support to Transport.
>
> It's also structured such that it's easy to add
> HTTP pipelining in the future.  (each persistent
> connection has a writer goroutine and reader
> goroutine)
>
> I won't submit this until it's further documented
> and tested, but it's ready to start being reviewed
> now.
>
> Please review this at http://codereview.appspot.com/4272045/
>
> Affected files:
>  M src/pkg/http/client.go
>  M src/pkg/http/proxy_test.go
>  M src/pkg/http/serve_test.go
>  M src/pkg/http/transport.go
>  M src/pkg/http/transport_test.go
>
>
>

[rsc, 2011-03-14 23:04:45]

ClientConn is meant for use in implementing
proxies and other programs that care about managing
which requests go out on which connections.
I haven't had a chance to look at the new Transport
yet, but would it make sense to use inside the implementation
of Transport even if it's not a replacement for it?

I don't understand why Transport would try to do
two simultaneous reads if it knew what was pending
on each connection.

Russ

[bradfitzgoog, 2011-03-14 23:20:26]

Okay, I'll go re-read the ClientConn code and try to make it work.

On Mon, Mar 14, 2011 at 4:04 PM, Russ Cox <rsc@golang.org> wrote:

> ClientConn is meant for use in implementing
> proxies and other programs that care about managing
> which requests go out on which connections.
> I haven't had a chance to look at the new Transport
> yet, but would it make sense to use inside the implementation
> of Transport even if it's not a replacement for it?
>
> I don't understand why Transport would try to do
> two simultaneous reads if it knew what was pending
> on each connection.
>
> Russ
>

[petar-m, 2011-03-14 23:53:00]

I am travelling and can't go in detail, BUT:

ClientConn is thread safe now. You are reading a stale comment we forgot.

You can see that (in persist.go) Reads and Writes are protected by
textproto pipeline locks.

--Petr


On 14 March 2011 19:20, Brad Fitzpatrick <bradfitz@google.com> wrote:
> Okay, I'll go re-read the ClientConn code and try to make it work.
>
> On Mon, Mar 14, 2011 at 4:04 PM, Russ Cox <rsc@golang.org> wrote:
>>
>> ClientConn is meant for use in implementing
>> proxies and other programs that care about managing
>> which requests go out on which connections.
>> I haven't had a chance to look at the new Transport
>> yet, but would it make sense to use inside the implementation
>> of Transport even if it's not a replacement for it?
>>
>> I don't understand why Transport would try to do
>> two simultaneous reads if it knew what was pending
>> on each connection.
>>
>> Russ
>
>

[petar-m, 2011-03-15 01:06:20]

So I had a chance to look slightly more carefully at your Transport stuff.

In short: You should use ClientConn.

(1) ClientConn is thread safe (the comment you read must be stale)

(2) ClientConn handles pipelining at a low-level so you can do
read/write without
thinking about this. You should not re-implement this.

(3) From looking at your code, Transport seems to be trying to be a more
intelligent layer that knows if you are using a proxy or not, etc.
It is definitely something that belongs on top of ClientConn. The logic
of pipelining has little to do with whether you are using proxies or not,
which is why it makes sense for them to be separate modules.

Please, read and understand the code of ClientConn. Also, this is
very tricky code which has been fine-tuned for a while. Not to mention
that Russ and I went through a few iterations and designs until
we converged to the simplest modular way of implementing this.

--P



On 14 March 2011 19:53, Petar Maymounkov <petarm@gmail.com> wrote:
> I am travelling and can't go in detail, BUT:
>
> ClientConn is thread safe now. You are reading a stale comment we forgot.
>
> You can see that (in persist.go) Reads and Writes are protected by
> textproto pipeline locks.
>
> --Petr
>
>
> On 14 March 2011 19:20, Brad Fitzpatrick <bradfitz@google.com> wrote:
>> Okay, I'll go re-read the ClientConn code and try to make it work.
>>
>> On Mon, Mar 14, 2011 at 4:04 PM, Russ Cox <rsc@golang.org> wrote:
>>>
>>> ClientConn is meant for use in implementing
>>> proxies and other programs that care about managing
>>> which requests go out on which connections.
>>> I haven't had a chance to look at the new Transport
>>> yet, but would it make sense to use inside the implementation
>>> of Transport even if it's not a replacement for it?
>>>
>>> I don't understand why Transport would try to do
>>> two simultaneous reads if it knew what was pending
>>> on each connection.
>>>
>>> Russ
>>
>>
>

[rsc, 2011-03-15 17:28:22]

Looked at the code briefly.  I think Petar is correct
that some of it would get simpler with ClientConn.

Russ

[bradfitz, 2011-03-18 00:40:40]

Petar,

I mostly re-did the patch with ClientConn.  (not finished or uploaded)
 While parts got significantly shorter, I ran into a design point I didn't
like:

ClientConn doesn't have goroutine sitting there always waiting on a read, so
how do you reliably detect when the remote side closes prematurely on you?
 By the time you Write to it seems too late to find out.  You'd need to be
able to clearly tell apart the difference between a write that guaranteed
not to have hit the remote side (so you know it's safe to retry) vs. one
that failed for other reasons.   How do you deal with that in your code?

Also because you don't have a long-lived read going on, you can't detect
un-solicited / maliciously injected fake responses, can you?  That is, if
you send 1 http request and immediately get 2 responses back, you don't know
the 2nd response is bogus.

And you still rely on higher levels to comply with the "SHOULD NOT" in RFC
2616 8.1.2.2 about not pipelining non-idempotent requests, right?  It seems
that ClientTransport could at least do that for you.

I'm still tempted to stay with my previous design.  I want your thoughts
before I decide, though.  Some of the points above are relatively minor and
I could live without but I don't like the idea of writing a request when
we've already heard the other side disconnect.

Btw, I'm no stranger to http.  I wrote LiveJournal's load balancer (perlbal)
that's still in wide use and saw every failure mode possible on both the
client & server side.  I'm pretty paranoid about getting it correct.

- Brad


On Mon, Mar 14, 2011 at 6:06 PM, Petar Maymounkov <petarm@gmail.com> wrote:

> So I had a chance to look slightly more carefully at your Transport stuff.
>
> In short: You should use ClientConn.
>
> (1) ClientConn is thread safe (the comment you read must be stale)
>
> (2) ClientConn handles pipelining at a low-level so you can do
> read/write without
> thinking about this. You should not re-implement this.
>
> (3) From looking at your code, Transport seems to be trying to be a more
> intelligent layer that knows if you are using a proxy or not, etc.
> It is definitely something that belongs on top of ClientConn. The logic
> of pipelining has little to do with whether you are using proxies or not,
> which is why it makes sense for them to be separate modules.
>
> Please, read and understand the code of ClientConn. Also, this is
> very tricky code which has been fine-tuned for a while. Not to mention
> that Russ and I went through a few iterations and designs until
> we converged to the simplest modular way of implementing this.
>
> --P
>
>
>
> On 14 March 2011 19:53, Petar Maymounkov <petarm@gmail.com> wrote:
> > I am travelling and can't go in detail, BUT:
> >
> > ClientConn is thread safe now. You are reading a stale comment we forgot.
> >
> > You can see that (in persist.go) Reads and Writes are protected by
> > textproto pipeline locks.
> >
> > --Petr
> >
> >
> > On 14 March 2011 19:20, Brad Fitzpatrick <bradfitz@google.com> wrote:
> >> Okay, I'll go re-read the ClientConn code and try to make it work.
> >>
> >> On Mon, Mar 14, 2011 at 4:04 PM, Russ Cox <rsc@golang.org> wrote:
> >>>
> >>> ClientConn is meant for use in implementing
> >>> proxies and other programs that care about managing
> >>> which requests go out on which connections.
> >>> I haven't had a chance to look at the new Transport
> >>> yet, but would it make sense to use inside the implementation
> >>> of Transport even if it's not a replacement for it?
> >>>
> >>> I don't understand why Transport would try to do
> >>> two simultaneous reads if it knew what was pending
> >>> on each connection.
> >>>
> >>> Russ
> >>
> >>
> >
>

[petar-m, 2011-03-18 14:50:44]

This is not a shortcoming of the ClientConn design.

ClientConn is designed around the principles of POSIX read/write interfaces.

The job of ClientConn is to give you a function that reads. It's job is not
to be extra smart and do continuous polling for a Read. You can do this
yourself if you need it. If this was a part of ClientConn, another user who
doesn't want a continuous read will be at a loss.

And yes, the RFC idempotent policies belong in a higher-level "smarter" class.

The idempotent stuff you are mentioning are not the only policy subtleties in
the RFC. In general, HTTP implementations have pluggable "Policy" classes
which sit on top of something like ClientConn and implement the more subtle
HTTP rules. Since there is so much complexity and non-compliance in the
policy part of HTTP, it is better to keep them separate from the core functions
so that people can write different Policy classes depending on their needs.

Entirely separately, I don't see why you need a continuous read loop.
After you make a Write, you should call a Read, which blocks until
something comes on the Wire. If the Response is malicious you should
detect this from its contents, not from timing information, which is not
precise anyway.

--P


On 17 March 2011 20:40, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Petar,
> I mostly re-did the patch with ClientConn.  (not finished or uploaded)
>  While parts got significantly shorter, I ran into a design point I didn't
> like:
>
> ClientConn doesn't have goroutine sitting there always waiting on a read, so
> how do you reliably detect when the remote side closes prematurely on you?
>  By the time you Write to it seems too late to find out.  You'd need to be
> able to clearly tell apart the difference between a write that guaranteed
> not to have hit the remote side (so you know it's safe to retry) vs. one
> that failed for other reasons.   How do you deal with that in your code?
> Also because you don't have a long-lived read going on, you can't detect
> un-solicited / maliciously injected fake responses, can you?  That is, if
> you send 1 http request and immediately get 2 responses back, you don't know
> the 2nd response is bogus.
> And you still rely on higher levels to comply with the "SHOULD NOT" in RFC
> 2616 8.1.2.2 about not pipelining non-idempotent requests, right?  It seems
> that ClientTransport could at least do that for you.
> I'm still tempted to stay with my previous design.  I want your thoughts
> before I decide, though.  Some of the points above are relatively minor and
> I could live without but I don't like the idea of writing a request when
> we've already heard the other side disconnect.
> Btw, I'm no stranger to http.  I wrote LiveJournal's load balancer (perlbal)
> that's still in wide use and saw every failure mode possible on both the
> client & server side.  I'm pretty paranoid about getting it correct.
> - Brad
>
> On Mon, Mar 14, 2011 at 6:06 PM, Petar Maymounkov <petarm@gmail.com> wrote:
>>
>> So I had a chance to look slightly more carefully at your Transport stuff.
>>
>> In short: You should use ClientConn.
>>
>> (1) ClientConn is thread safe (the comment you read must be stale)
>>
>> (2) ClientConn handles pipelining at a low-level so you can do
>> read/write without
>> thinking about this. You should not re-implement this.
>>
>> (3) From looking at your code, Transport seems to be trying to be a more
>> intelligent layer that knows if you are using a proxy or not, etc.
>> It is definitely something that belongs on top of ClientConn. The logic
>> of pipelining has little to do with whether you are using proxies or not,
>> which is why it makes sense for them to be separate modules.
>>
>> Please, read and understand the code of ClientConn. Also, this is
>> very tricky code which has been fine-tuned for a while. Not to mention
>> that Russ and I went through a few iterations and designs until
>> we converged to the simplest modular way of implementing this.
>>
>> --P
>>
>>
>>
>> On 14 March 2011 19:53, Petar Maymounkov <petarm@gmail.com> wrote:
>> > I am travelling and can't go in detail, BUT:
>> >
>> > ClientConn is thread safe now. You are reading a stale comment we
>> > forgot.
>> >
>> > You can see that (in persist.go) Reads and Writes are protected by
>> > textproto pipeline locks.
>> >
>> > --Petr
>> >
>> >
>> > On 14 March 2011 19:20, Brad Fitzpatrick <bradfitz@google.com> wrote:
>> >> Okay, I'll go re-read the ClientConn code and try to make it work.
>> >>
>> >> On Mon, Mar 14, 2011 at 4:04 PM, Russ Cox <rsc@golang.org> wrote:
>> >>>
>> >>> ClientConn is meant for use in implementing
>> >>> proxies and other programs that care about managing
>> >>> which requests go out on which connections.
>> >>> I haven't had a chance to look at the new Transport
>> >>> yet, but would it make sense to use inside the implementation
>> >>> of Transport even if it's not a replacement for it?
>> >>>
>> >>> I don't understand why Transport would try to do
>> >>> two simultaneous reads if it knew what was pending
>> >>> on each connection.
>> >>>
>> >>> Russ
>> >>
>> >>
>> >
>
>

[rsc, 2011-03-18 23:42:57]

> The job of ClientConn is to give you a function that reads. It's job is not
> to be extra smart and do continuous polling for a Read. You can do this
> yourself if you need it. If this was a part of ClientConn, another user who
> doesn't want a continuous read will be at a loss.

Yes, it's true that a user who didn't want a continuous read would
be at a loss.  But a user who does want a continous read can't
do it with ClientConn, because ClientConn insists on knowing
which request it should be reading the response for.

I think the way Transport is managing the connection is just a different
approach from the way ClientConn does.  There are valid design reasons
to choose either, and I think the design reasons behind Transport and
the ways it wants to manage connections don't fit the assumptions that
ClientConn makes, even though that wasn't apparent at first glance.
That's fine: Transport doesn't have to use ClientConn.

Russ

[bradfitz, 2011-03-19 01:43:32]

I'm going to try to finish my ClientConn version.

ClientConn lets me pass in the bufio.Reader so I should be able to still
have my background read (of a single byte) and when input from the client is
expected just unread the byte to the bufio.Reader (and then ClientConn.Read)
but when input is unexpected (including an unexpected/timeout EOF from the
server), simply close it down, ideally in time to prevent any ill-fated
Write on that socket.

I may not find time this weekend but I'll work on this as soon as I can.
 I'd prefer to reuse code if possible.

On Fri, Mar 18, 2011 at 4:42 PM, Russ Cox <rsc@golang.org> wrote:

> > The job of ClientConn is to give you a function that reads. It's job is
> not
> > to be extra smart and do continuous polling for a Read. You can do this
> > yourself if you need it. If this was a part of ClientConn, another user
> who
> > doesn't want a continuous read will be at a loss.
>
> Yes, it's true that a user who didn't want a continuous read would
> be at a loss.  But a user who does want a continous read can't
> do it with ClientConn, because ClientConn insists on knowing
> which request it should be reading the response for.
>
> I think the way Transport is managing the connection is just a different
> approach from the way ClientConn does.  There are valid design reasons
> to choose either, and I think the design reasons behind Transport and
> the ways it wants to manage connections don't fit the assumptions that
> ClientConn makes, even though that wasn't apparent at first glance.
> That's fine: Transport doesn't have to use ClientConn.
>
> Russ
>

[r, 2011-03-19 03:47:51]

ClientConns to the left of them, ClientConns to the right of them.
Into the valley of death rode the 404s.

-rob

[bradfitz, 2011-03-23 00:24:38]

Hello rsc, petar-m, bradfitzwork, r (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2011-03-23 00:27:22]

I modified the $GOROOT/.hg/codereview/cl.4272045 file to modify the CL text
but it got reverted.  And hg mail's -m is only documented for working on new
changes.

I guess the web UI's the only way to modify the text.

Anyway, ready for review now.


On Tue, Mar 22, 2011 at 5:24 PM, <bradfitz@golang.org> wrote:

> Hello rsc, petar-m, bradfitzwork, r (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
> http://codereview.appspot.com/4272045/
>

[rsc, 2011-03-23 01:36:20]

On Tue, Mar 22, 2011 at 20:27, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> I modified the $GOROOT/.hg/codereview/cl.4272045 file to modify the CL text
> but it got reverted.  And hg mail's -m is only documented for working on new
> changes.

You can run hg change 4272045 to edit the CL description
(or the file, review, and cc lists).  The master is the one on
the web but hg change updates it.

Russ

[rsc, 2011-03-23 13:42:18]

LGTM

http://codereview.appspot.com/4272045/diff/14001/src/pkg/http/export_test.go
File src/pkg/http/export_test.go (right):

http://codereview.appspot.com/4272045/diff/14001/src/pkg/http/export_test.go#...
src/pkg/http/export_test.go:10: func (t *Transport) GetIdleConnKeysForTesting()
(keys []string) {
s/Get//
http://goneat.org/doc/effective_go.html#Getters

http://codereview.appspot.com/4272045/diff/14001/src/pkg/http/httptest/server.go
File src/pkg/http/httptest/server.go (right):

http://codereview.appspot.com/4272045/diff/14001/src/pkg/http/httptest/server...
src/pkg/http/httptest/server.go:12: "os"
sort down

http://codereview.appspot.com/4272045/diff/14001/src/pkg/http/transport.go
File src/pkg/http/transport.go (right):

http://codereview.appspot.com/4272045/diff/14001/src/pkg/http/transport.go#ne...
src/pkg/http/transport.go:134: // is malformed, an error is returned
comment out of date

http://codereview.appspot.com/4272045/diff/14001/src/pkg/http/transport.go#ne...
src/pkg/http/transport.go:149: func (t *Transport) returnIdleConn(pconn
*persistConn) {
makes it sound like this function returns an idleConn.

putIdleConn maybe?

http://codereview.appspot.com/4272045/diff/14001/src/pkg/http/transport.go#ne...
src/pkg/http/transport.go:271: // matchNoProxy returns true if requests to addr
should not use a proxy,
please invert this to useProxy, which will be easier
to read at the call sites.  i know it's not your name.

http://codereview.appspot.com/4272045/diff/14001/src/pkg/http/transport.go#ne...
src/pkg/http/transport.go:338: func (cm *connectMethod) hostToTLSVerify() string
{
sounds like a conversion to "TLSVerify".
tlsHost ?

[bradfitzgoog, 2011-03-23 17:35:00]

Done.

Submitting & waiting to see what I've broken.   (I suspect FreeBSD at least,
for no particular reason....)

On Wed, Mar 23, 2011 at 6:42 AM, <rsc@golang.org> wrote:

> LGTM
>
>
>
>
> http://codereview.appspot.com/4272045/diff/14001/src/pkg/http/export_test.go
> File src/pkg/http/export_test.go (right):
>
>
>
http://codereview.appspot.com/4272045/diff/14001/src/pkg/http/export_test.go#...
> src/pkg/http/export_test.go:10: func (t *Transport)
> GetIdleConnKeysForTesting() (keys []string) {
> s/Get//
> http://goneat.org/doc/effective_go.html#Getters
>
>
>
http://codereview.appspot.com/4272045/diff/14001/src/pkg/http/httptest/server.go
> File src/pkg/http/httptest/server.go (right):
>
>
>
http://codereview.appspot.com/4272045/diff/14001/src/pkg/http/httptest/server...
> src/pkg/http/httptest/server.go:12: "os"
> sort down
>
> http://codereview.appspot.com/4272045/diff/14001/src/pkg/http/transport.go
> File src/pkg/http/transport.go (right):
>
>
>
http://codereview.appspot.com/4272045/diff/14001/src/pkg/http/transport.go#ne...
> src/pkg/http/transport.go:134: // is malformed, an error is returned
> comment out of date
>
>
>
http://codereview.appspot.com/4272045/diff/14001/src/pkg/http/transport.go#ne...
> src/pkg/http/transport.go:149: func (t *Transport) returnIdleConn(pconn
> *persistConn) {
> makes it sound like this function returns an idleConn.
>
> putIdleConn maybe?
>
>
>
http://codereview.appspot.com/4272045/diff/14001/src/pkg/http/transport.go#ne...
> src/pkg/http/transport.go:271: // matchNoProxy returns true if requests
> to addr should not use a proxy,
> please invert this to useProxy, which will be easier
> to read at the call sites.  i know it's not your name.
>
>
>
http://codereview.appspot.com/4272045/diff/14001/src/pkg/http/transport.go#ne...
> src/pkg/http/transport.go:338: func (cm *connectMethod)
> hostToTLSVerify() string {
> sounds like a conversion to "TLSVerify".
> tlsHost ?
>
>
> http://codereview.appspot.com/4272045/
>

[rsc, 2011-03-23 17:35:30]

> Submitting & waiting to see what I've broken.   (I suspect FreeBSD at least,
> for no particular reason....)

Beat you to it.

[bradfitz, 2011-03-23 17:38:26]

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

http: export Transport, add keep-alive support

This patch adds a connection cache and keep-alive
support to Transport, which is used by the
HTTP client.

It's also structured such that it's easy to add
HTTP pipelining in the future.

R=rsc, petar-m, bradfitzwork, r
CC=golang-dev
http://codereview.appspot.com/4272045

[petar-m, 2011-03-23 17:41:38]

Just got a chance to look at this. Looks good ;)

--P


On 23 March 2011 13:38,  <bradfitz@golang.org> wrote:
> *** Submitted as
> http://code.google.com/p/go/source/detail?r=ffbe9af96475 ***
>
> http: export Transport, add keep-alive support
>
> This patch adds a connection cache and keep-alive
> support to Transport, which is used by the
> HTTP client.
>
> It's also structured such that it's easy to add
> HTTP pipelining in the future.
>
> R=rsc, petar-m, bradfitzwork, r
> CC=golang-dev
> http://codereview.appspot.com/4272045
>
>
> http://codereview.appspot.com/4272045/
>