code review 4239044: http: expose Client's Transport

http: expose Client's Transport

This is again an intentionally minimal change.

The plan is to keep Client's zero value be a usable
client, with optional fields being added over time
(e.g. cookie manager, redirect policy, auth)

[bradfitz, 2011-02-25 22:41:44]

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

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

[rsc, 2011-02-25 23:19:20]

Adding unused int doesn't do very much.
You can still write:

x := new(http.Client)
x.Transport = t

or

x := &http.Client{Transport: t}

Is the Client going to subsume the ClientConfig,
moving the config fields into Client directly?
That seems fine, I'm just curious about the plan.

Russ

On Fri, Feb 25, 2011 at 17:41,  <bradfitz@golang.org> wrote:
> Reviewers: rsc,
>
> Message:
> Hello rsc (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> http: allow creating custom Clients
>
> This is again an intentionally minimal change.  I expect
> Client to grow and don't want it directly instantiable.
> Further, I don't want the NewClient constructor to require all
> its fields (Transport, ClientConfig) as it will be incomplete
> in the future.
>
> I figure we can do ClientConfig and friends later just by adding
> new fields to Client with the zero value defaulting to current
> behavior.
>
> Please review this at http://codereview.appspot.com/4239044/
>
> Affected files:
>  M src/pkg/http/client.go
>  M src/pkg/http/client_test.go
>
>
> Index: src/pkg/http/client.go
> ===================================================================
> --- a/src/pkg/http/client.go
> +++ b/src/pkg/http/client.go
> @@ -19,12 +19,19 @@
>  // A Client is an HTTP client.
>  // It is not yet possible to create custom Clients; use DefaultClient.
>  type Client struct {
> -       transport ClientTransport // if nil, DefaultTransport is used
> +       Transport ClientTransport // if nil, DefaultTransport is used
> +
> +       unused int // prevent instantiation; there will likely be internal
> state in the future
>  }
>
>  // DefaultClient is the default Client and is used by Get, Head, and Post.
>  var DefaultClient = &Client{}
>
> +// NewClient returns a new Client which you can then initialize as needed.
> +func NewClient() *Client {
> +       return &Client{}
> +}
> +
>  // ClientTransport is an interface representing the ability to execute a
>  // single HTTP transaction, obtaining the Response for a given Request.
>  type ClientTransport interface {
> @@ -91,7 +98,7 @@
>  //
>  // Generally Get, Post, or PostForm will be used instead of Do.
>  func (c *Client) Do(req *Request) (resp *Response, err os.Error) {
> -       return send(req, c.transport)
> +       return send(req, c.Transport)
>  }
>
>
> @@ -103,6 +110,10 @@
>  func send(req *Request, t ClientTransport) (resp *Response, err os.Error) {
>        if t == nil {
>                t = DefaultTransport
> +               if t == nil {
> +                       err = os.NewError("no http.Client.Transport or
> http.DefaultTransport")
> +                       return
> +               }
>        }
>        info := req.URL.RawUserinfo
>        if len(info) > 0 {
> @@ -183,7 +194,7 @@
>                        break
>                }
>                url = req.URL.String()
> -               if r, err = send(&req, c.transport); err != nil {
> +               if r, err = send(&req, c.Transport); err != nil {
>                        break
>                }
>                if shouldRedirect(r.StatusCode) {
> @@ -232,7 +243,7 @@
>                return nil, err
>        }
>
> -       return send(&req, c.transport)
> +       return send(&req, c.Transport)
>  }
>
>  // PostForm issues a POST to the specified URL,
> @@ -268,7 +279,7 @@
>                return nil, err
>        }
>
> -       return send(&req, c.transport)
> +       return send(&req, c.Transport)
>  }
>
>  // TODO: remove this function when PostForm takes a multimap.
> @@ -294,7 +305,7 @@
>        if req.URL, err = ParseURL(url); err != nil {
>                return
>        }
> -       return send(&req, c.transport)
> +       return send(&req, c.Transport)
>  }
>
>  type nopCloser struct {
> Index: src/pkg/http/client_test.go
> ===================================================================
> --- a/src/pkg/http/client_test.go
> +++ b/src/pkg/http/client_test.go
> @@ -51,7 +51,7 @@
>
>  func TestGetRequestFormat(t *testing.T) {
>        tr := &recordingTransport{}
> -       client := &Client{transport: tr}
> +       client := &Client{Transport: tr}
>        url := "http://dummy.faketld/"
>        client.Get(url) // Note: doesn't hit network
>        if tr.req.Method != "GET" {
>
>
>

[bradfitzgoog, 2011-02-25 23:39:37]

On Fri, Feb 25, 2011 at 3:19 PM, Russ Cox <rsc@golang.org> wrote:

> Adding unused int doesn't do very much.
> You can still write:
>
> x := new(http.Client)
> x.Transport = t
>
> or
>
> x := &http.Client{Transport: t}
>

Oh, right.  I was thinking of something else.  In which case I'll just
remove NewClient and let people initialize it directly.  Any private stuff I
may need (e.g. a sync.Mutex) will have an acceptable zero value.


> Is the Client going to subsume the ClientConfig,
> moving the config fields into Client directly?
> That seems fine, I'm just curious about the plan.
>

I'm starting to lean that way but wanted to write up a revised proposal
before I convince myself for sure.

I worry about big interfaces that encourage growth and breakages.

I'd rather hang a bunch of nil-able interfaces (AuthConfig, CookieConfig,
RedirectConfig) off Client.  That will allow us to grow in the future
without breaking people's code.

Regardless of what path we take, though, exposing Client.Transport seems
like a safe first step.

[rsc, 2011-02-28 15:09:10]

On Fri, Feb 25, 2011 at 18:39, Brad Fitzpatrick <bradfitz@google.com> wrote:
> On Fri, Feb 25, 2011 at 3:19 PM, Russ Cox <rsc@golang.org> wrote:
>>
>> Adding unused int doesn't do very much.
>> You can still write:
>>
>> x := new(http.Client)
>> x.Transport = t
>>
>> or
>>
>> x := &http.Client{Transport: t}
>
> Oh, right.  I was thinking of something else.  In which case I'll just
> remove NewClient and let people initialize it directly.  Any private stuff I
> may need (e.g. a sync.Mutex) will have an acceptable zero value.
>
>>
>> Is the Client going to subsume the ClientConfig,
>> moving the config fields into Client directly?
>> That seems fine, I'm just curious about the plan.
>
> I'm starting to lean that way but wanted to write up a revised proposal
> before I convince myself for sure.
> I worry about big interfaces that encourage growth and breakages.
> I'd rather hang a bunch of nil-able interfaces (AuthConfig, CookieConfig,
> RedirectConfig) off Client.

Sure, that seems reasonable.  Basically the same fields
that were going to be in ClientConfig, just in Client instead.

Russ

[bradfitz, 2011-03-01 21:46:25]

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

Please take another look.

[bradfitz, 2011-03-01 21:50:12]

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

Please take another look.

[rsc, 2011-03-01 21:57:50]

LGTM

[bradfitz, 2011-03-01 22:07:35]

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

http: expose Client's Transport

This is again an intentionally minimal change.

The plan is to keep Client's zero value be a usable
client, with optional fields being added over time
(e.g. cookie manager, redirect policy, auth)

R=rsc
CC=golang-dev
http://codereview.appspot.com/4239044