code review 6653049: net/http/client.go: fix cookie handling on (*Client) Do()

net/http/client.go: fix cookie handling on (*Client) Do() Fix the problem with no cookie handling when sending other than GET or HEAD request through (*Client) Do(*Request) (*Resposne, error). https://code.google.com/p/go/issues/detail?id=3985 Adds a function (*Client) send(*Request) (*Reponse, error): - sets cookies from CookieJar to request, - sends request - parses a reply cookies and updates CookieJar

[pawelszczur, 2012-10-12 15:36:09]

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

[pawelszczur, 2012-10-18 08:47:20]

Ping, https://codereview.appspot.com/6653049/

[gobot, 2012-10-22 21:41:54]

R=bradfitz (assigned by bradfitz)

[bradfitz, 2012-10-29 11:31:51]

Sorry for the slow review. Kick me in the future if you want my attention. https://codereview.appspot.com/6653049/diff/2001/src/pkg/net/http/client.go File src/pkg/net/http/client.go (right): https://codereview.appspot.com/6653049/diff/2001/src/pkg/net/http/client.go#n... src/pkg/net/http/client.go:90: func handleCookiesAndSend(jar CookieJar, req *Request, transport RoundTripper) (resp *Response, err error) { since CookieJar and RoundTripper both always come from the Client anyway, maybe just make this a private method on *Client? func (c *Client) cookieSend(req *Request) (*Response, error) (and I have a slight preference for not naming the result parameters, for clarity in the return statements below) https://codereview.appspot.com/6653049/diff/2001/src/pkg/net/http/client.go#n... src/pkg/net/http/client.go:97: if err == nil && jar != nil { more idiomatic would be to return early on error. if err != nil { return nil, err } https://codereview.appspot.com/6653049/diff/2001/src/pkg/net/http/client.go#n... src/pkg/net/http/client.go:98: jar.SetCookies(req.URL, resp.Cookies()) most the time this jar will be a blackHoleJar, which means we waste time parsing cookies in resp.Cookies() just to throw them away, so maybe we should ditch blackHoleJar, once this is a method on Client and we have the jar == nil checks anyway https://codereview.appspot.com/6653049/diff/2001/src/pkg/net/http/client.go#n... src/pkg/net/http/client.go:233: jar = blackHoleJar{} perhaps we should get rid of this now

[pawelszczur, 2012-10-29 14:38:36]

Hello bradfitz@golang.org (cc: gobot@golang.org, golang-dev@googlegroups.com), Please take another look.

[pawelszczur, 2012-10-29 14:41:23]

Hello bradfitz@golang.org (cc: gobot@golang.org, golang-dev@googlegroups.com), Please take another look.

[bradfitz, 2012-10-29 14:46:57]

https://codereview.appspot.com/6653049/diff/13001/src/pkg/net/http/client.go File src/pkg/net/http/client.go (left): https://codereview.appspot.com/6653049/diff/13001/src/pkg/net/http/client.go#... src/pkg/net/http/client.go:220: jar = blackHoleJar{} this was the only use of blackHoleJar, so you can delete it from jar.go now. be sure to add jar.go to this CL, with "hg change nnnnnn" and then add it to the file list and save before you hg mail again. https://codereview.appspot.com/6653049/diff/13001/src/pkg/net/http/client.go File src/pkg/net/http/client.go (right): https://codereview.appspot.com/6653049/diff/13001/src/pkg/net/http/client.go#... src/pkg/net/http/client.go:90: func (c *Client) handleCookiesAndSend(req *Request) (resp *Response, err error) { let's just call this method "send". it's internal. no need to have the callers list out everything it does. also, drop the named result parameters. just (*Response, error)

[pawelszczur, 2012-10-29 16:38:26]

Hello bradfitz@golang.org (cc: gobot@golang.org, golang-dev@googlegroups.com), Please take another look.

[pawelszczur, 2012-10-29 16:39:00]

https://codereview.appspot.com/6653049/diff/2001/src/pkg/net/http/client.go File src/pkg/net/http/client.go (right): https://codereview.appspot.com/6653049/diff/2001/src/pkg/net/http/client.go#n... src/pkg/net/http/client.go:90: func handleCookiesAndSend(jar CookieJar, req *Request, transport RoundTripper) (resp *Response, err error) { On 2012/10/29 11:31:51, bradfitz wrote: > since CookieJar and RoundTripper both always come from the Client anyway, maybe > just make this a private method on *Client? > > func (c *Client) cookieSend(req *Request) (*Response, error) > > (and I have a slight preference for not naming the result parameters, for > clarity in the return statements below) Thanks for pointing that. https://codereview.appspot.com/6653049/diff/2001/src/pkg/net/http/client.go#n... src/pkg/net/http/client.go:97: if err == nil && jar != nil { On 2012/10/29 11:31:51, bradfitz wrote: > more idiomatic would be to return early on error. > > if err != nil { > return nil, err > } Done. https://codereview.appspot.com/6653049/diff/2001/src/pkg/net/http/client.go#n... src/pkg/net/http/client.go:98: jar.SetCookies(req.URL, resp.Cookies()) On 2012/10/29 11:31:51, bradfitz wrote: > most the time this jar will be a blackHoleJar, which means we waste time parsing > cookies in resp.Cookies() just to throw them away, so maybe we should ditch > blackHoleJar, once this is a method on Client and we have the jar == nil checks > anyway Done. https://codereview.appspot.com/6653049/diff/2001/src/pkg/net/http/client.go#n... src/pkg/net/http/client.go:233: jar = blackHoleJar{} On 2012/10/29 11:31:51, bradfitz wrote: > perhaps we should get rid of this now Done. https://codereview.appspot.com/6653049/diff/13001/src/pkg/net/http/client.go File src/pkg/net/http/client.go (left): https://codereview.appspot.com/6653049/diff/13001/src/pkg/net/http/client.go#... src/pkg/net/http/client.go:220: jar = blackHoleJar{} On 2012/10/29 14:46:57, bradfitz wrote: > this was the only use of blackHoleJar, so you can delete it from jar.go now. > > be sure to add jar.go to this CL, with "hg change nnnnnn" and then add it to the > file list and save before you hg mail again. Done. https://codereview.appspot.com/6653049/diff/13001/src/pkg/net/http/client.go File src/pkg/net/http/client.go (right): https://codereview.appspot.com/6653049/diff/13001/src/pkg/net/http/client.go#... src/pkg/net/http/client.go:90: func (c *Client) handleCookiesAndSend(req *Request) (resp *Response, err error) { So there will be 2 methods called "send". One on *Client and a stand alone one. That can be quite misleading? Done. On 2012/10/29 14:46:57, bradfitz wrote: > let's just call this method "send". it's internal. no need to have the callers > list out everything it does. > > also, drop the named result parameters. just (*Response, error)

[pawelszczur, 2012-10-29 16:39:41]

Hello bradfitz@golang.org (cc: gobot@golang.org, golang-dev@googlegroups.com), Please take another look.

[pawelszczur, 2012-10-29 16:39:58]

Hello bradfitz@golang.org (cc: gobot@golang.org, golang-dev@googlegroups.com), Please take another look.

[bradfitz, 2012-10-29 16:41:40]

LGTM Thanks! On Mon, Oct 29, 2012 at 5:39 PM, <filemon@google.com> wrote: > Hello bradfitz@golang.org (cc: gobot@golang.org, > golang-dev@googlegroups.com), > > Please take another look. > > > http://codereview.appspot.com/**6653049/<http://codereview.appspot.com/6653049/> > 

[bradfitz, 2012-10-29 16:56:35]

*** Submitted as http://code.google.com/p/go/source/detail?r=d2f3c74b9998 *** net/http/client.go: fix cookie handling on (*Client) Do() Fix the problem with no cookie handling when sending other than GET or HEAD request through (*Client) Do(*Request) (*Resposne, error). https://code.google.com/p/go/issues/detail?id=3985 Adds a function (*Client) send(*Request) (*Reponse, error): - sets cookies from CookieJar to request, - sends request - parses a reply cookies and updates CookieJar Fixes issue 3985 R=bradfitz CC=gobot, golang-dev http://codereview.appspot.com/6653049 Committer: Brad Fitzpatrick <bradfitz@golang.org>

[serbaut, 2012-12-19 16:45:19]

https://codereview.appspot.com/6653049/diff/12002/src/pkg/net/http/client.go File src/pkg/net/http/client.go (right): https://codereview.appspot.com/6653049/diff/12002/src/pkg/net/http/client.go#... src/pkg/net/http/client.go:101: c.Jar.SetCookies(req.URL, resp.Cookies()) SetCookies is always called here, even if there are no cookies in resp. Shouldn't it be (as before): if c.Jar != nil { if rc := resp.Cookies(); len(rc) > 0 { c.Jar.SetCookies(req.URL, rc) } } ?

[bradfitz, 2012-12-19 17:04:02]

https://codereview.appspot.com/6653049/diff/12002/src/pkg/net/http/client.go File src/pkg/net/http/client.go (right): https://codereview.appspot.com/6653049/diff/12002/src/pkg/net/http/client.go#... src/pkg/net/http/client.go:101: c.Jar.SetCookies(req.URL, resp.Cookies()) On 2012/12/19 16:45:19, serbaut wrote: > SetCookies is always called here, even if there are no cookies in resp. > Shouldn't it be (as before): > > if c.Jar != nil { > if rc := resp.Cookies(); len(rc) > 0 { > c.Jar.SetCookies(req.URL, rc) > } > } > > ? That would also be fine. Feel free to send a CL.