code review 4182086: http: introduce start of Client and ClientTransport

http: introduce start of Client and ClientTransport

Much yet to come, but this is a safe first step, introducing
an in-the-future configurable Client object (where policy for
cookies, auth, redirects will live) as well as introducing a
ClientTransport interface for sending requests.

The CL intentionally ignores everything around the creation
and configuration of Clients and merely ports/wraps the old
interfaces to/around Client/ClientTransport.

[bradfitz, 2011-02-22 05:40:59]

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

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

[bradfitz, 2011-02-22 05:48:38]

Note: this CL contains almost no new code.

In particular I just moved the old http/client.go's send() to
http/transport.go's Do().  The rest are some wrappers and default globals.

This should be flexible enough for future http plans, though, and we can add
the remaining parts in incremental CLs.

On Mon, Feb 21, 2011 at 9:40 PM, <bradfitz@golang.org> wrote:

> Reviewers: rsc, dsymonds, nigeltao,
>
> Message:
> Hello rsc, dsymonds, nigeltao (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> http: introduce start of Client and ClientTransport
>
> Much yet to come, but this is a safe first step, introducing
> an in-the-future configurable Client object (where policy for
> cookies, auth, redirects will live) as well as introducing a
> ClientTransport interface for sending requests.
>
> The CL intentionally ignores everything around the creation
> and configuration of Clients and merely ports/wraps the old
> interfaces to/around Client/ClientTransport.
>
> Please review this at http://codereview.appspot.com/4182086/
>
> Affected files:
>  M src/pkg/http/Makefile
>  M src/pkg/http/client.go
>  M src/pkg/http/fs_test.go
>  A src/pkg/http/transport.go
>
>
>

[bradfitzgoog, 2011-02-23 00:18:37]

Any thoughts on this?

I'll re-sync this on top of petarm's http Header/textproto change once that
goes in but see anything besides that?


On Mon, Feb 21, 2011 at 9:48 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> Note: this CL contains almost no new code.
>
> In particular I just moved the old http/client.go's send() to
> http/transport.go's Do().  The rest are some wrappers and default globals.
>
> This should be flexible enough for future http plans, though, and we can
> add the remaining parts in incremental CLs.
>
> On Mon, Feb 21, 2011 at 9:40 PM, <bradfitz@golang.org> wrote:
>
>> Reviewers: rsc, dsymonds, nigeltao,
>>
>> Message:
>> Hello rsc, dsymonds, nigeltao (cc: golang-dev@googlegroups.com),
>>
>> I'd like you to review this change to
>> https://go.googlecode.com/hg/
>>
>>
>> Description:
>> http: introduce start of Client and ClientTransport
>>
>> Much yet to come, but this is a safe first step, introducing
>> an in-the-future configurable Client object (where policy for
>> cookies, auth, redirects will live) as well as introducing a
>> ClientTransport interface for sending requests.
>>
>> The CL intentionally ignores everything around the creation
>> and configuration of Clients and merely ports/wraps the old
>> interfaces to/around Client/ClientTransport.
>>
>> Please review this at http://codereview.appspot.com/4182086/
>>
>> Affected files:
>>  M src/pkg/http/Makefile
>>  M src/pkg/http/client.go
>>  M src/pkg/http/fs_test.go
>>  A src/pkg/http/transport.go
>>
>>
>>
>

[rsc, 2011-02-23 04:00:17]

On Tue, Feb 22, 2011 at 19:18, Brad Fitzpatrick <bradfitz@google.com> wrote:
> Any thoughts on this?
> I'll re-sync this on top of petarm's http Header/textproto change once that
> goes in but see anything besides that?

Plan to review tomorrow.
Still catching up from the long weekend.

[rsc, 2011-02-23 17:44:14]

Looks good.  I assume the next CL will add ClientConfig.

http://codereview.appspot.com/4182086/diff/3001/src/pkg/http/Makefile
File src/pkg/http/Makefile (right):

http://codereview.appspot.com/4182086/diff/3001/src/pkg/http/Makefile#newcode19
src/pkg/http/Makefile:19: transport.go\
|sort

http://codereview.appspot.com/4182086/diff/3001/src/pkg/http/client.go
File src/pkg/http/client.go (right):

http://codereview.appspot.com/4182086/diff/3001/src/pkg/http/client.go#newcode19
src/pkg/http/client.go:19: // Client is an HTTP client.  Currently custom
clients cannot be created
// A Client is an HTTP client.
// It is not yet possible to create custom Clients; use DefaultClient.

http://codereview.appspot.com/4182086/diff/3001/src/pkg/http/client.go#newcode25
src/pkg/http/client.go:25: // DefaultClient is a Client with sane default
settings.
// DefaultClient is the default Client and is used by Get, Head, and Post.

http://codereview.appspot.com/4182086/diff/3001/src/pkg/http/client.go#newcode28
src/pkg/http/client.go:28: // ClientTransport is an interface to send a receive
a single raw HTTP
// ClientTransport is an interface representing the ability to execute a
// single HTTP transaction, obtaining the Response for a given Request.

http://codereview.appspot.com/4182086/diff/3001/src/pkg/http/client.go#newcode32
src/pkg/http/client.go:32: // Do sends a single HTTP request and returns the
immediate
// Do executes a single HTTP transaction, returning the Response for the
// request req.  Do should not attempt to interpret the response.
// In particular, Do must return err == nil if it obtained a response,
// regardless of the response's HTTP status code.  A non-nil err should
// be reserved for failure to obtain a response.  Similarly, Do should
// not attempt to handle higher-level protocol details such as redirects,
// authentication, or cookies.

http://codereview.appspot.com/4182086/diff/3001/src/pkg/http/client.go#newcode40
src/pkg/http/client.go:40: // DefaultTransport is a default implementation of
ClientTransport.
// DefaultTransport is the default implementation of ClientTransport
// and is used by DefaultClient.  It establishes a new network connection for
// each call to Do and uses HTTP proxies as directed by the $HTTP_PROXY and
// $NO_PROXY (or $http_proxy and $no_proxy) environment variables.

http://codereview.appspot.com/4182086/diff/3001/src/pkg/http/client.go#newcode89
src/pkg/http/client.go:89: // Do sends an HTTP request and returns an HTTP
response, following
Hmm.  So this Do is different from the ClientTransport.Do
in that the latter must not deal with redirects and cookies and such.
Fine for now just something to keep in mind.

http://codereview.appspot.com/4182086/diff/3001/src/pkg/http/client.go#newcod...
src/pkg/http/client.go:131: // Get is a convenience wrapper around
http.DefaultClient.Get.
Should still start, as the original comment did, with what the
caller wants to know about how to use Get.

// Get issues a GET to the specified URL ...

The last sentence of the comment should be

//
// Get is a convenience wrapper around DefaultClient.Get.

so that users can go look at that next.  But the description
should be self-contained, so that you don't have to read
about DefaultClient to use Get.

same applies to the other comments.

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

http://codereview.appspot.com/4182086/diff/3001/src/pkg/http/transport.go#new...
src/pkg/http/transport.go:145: DefaultTransport = &transport{}
better to do in the declaration of DefaultTransport.
then if DefaultTransport isn't used it is easier for
the linker to see that the code is unnecessary.

[bradfitz, 2011-02-23 19:48:06]

http://codereview.appspot.com/4182086/diff/3001/src/pkg/http/client.go
File src/pkg/http/client.go (right):

http://codereview.appspot.com/4182086/diff/3001/src/pkg/http/client.go#newcode89
src/pkg/http/client.go:89: // Do sends an HTTP request and returns an HTTP
response, following
On 2011/02/23 17:44:15, rsc wrote:
> Hmm.  So this Do is different from the ClientTransport.Do
> in that the latter must not deal with redirects and cookies and such.
> Fine for now just something to keep in mind.

Could name it something else... "Send" or "Request" maybe.

[bradfitz, 2011-02-23 19:48:31]

Hello rsc, dsymonds, nigeltao, bradfitzwork (cc: golang-dev@googlegroups.com),

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

[rsc, 2011-02-23 19:56:21]

LGTM

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

http://codereview.appspot.com/4182086/diff/3001/src/pkg/http/transport.go#new...
src/pkg/http/transport.go:145: DefaultTransport = &transport{}
On 2011/02/23 17:44:15, rsc wrote:
> better to do in the declaration of DefaultTransport.
> then if DefaultTransport isn't used it is easier for
> the linker to see that the code is unnecessary.

missed this one

[bradfitz, 2011-02-23 20:13:47]

Hello rsc, dsymonds, nigeltao, bradfitzwork (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-02-23 20:15:36]

LGTM

[bradfitz, 2011-02-23 20:20:55]

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

http: introduce start of Client and ClientTransport

Much yet to come, but this is a safe first step, introducing
an in-the-future configurable Client object (where policy for
cookies, auth, redirects will live) as well as introducing a
ClientTransport interface for sending requests.

The CL intentionally ignores everything around the creation
and configuration of Clients and merely ports/wraps the old
interfaces to/around Client/ClientTransport.

R=rsc, dsymonds, nigeltao, bradfitzwork
CC=golang-dev
http://codereview.appspot.com/4182086

Committer: Brad Fitzpatrick <bradfitz@golang.org>