net/http: Client doesn't correctly set cookies for all requests - patch attached

[gopherbot]

by steven.hartland@multiplay.co.uk:

What steps will reproduce the problem?
If possible, include a link to a program on play.golang.org.
1. Using a working CookieJar (not the default blackHoleJar)
2. Do a request using the client api to a URL that sets a cookie
3. Do a POST request using the client API for which the above cookie is valid

What is the expected output?
The cookie returned from request in #1 should be set for #2


What do you see instead?
No cookies are set and no call to client.Jar.Cookies(url.URL) for the POST request

Which compiler are you using (5g, 6g, 8g, gccgo)?
6g

Which operating system are you using?
FreeBSD

Which version are you using?  (run 'go version')
go version go1

Please provide any additional information below.
The attached file is a patch from go1, which fixes this behaviour by delegating the
maintenance of cookies to the now client method send which under pins all actions.

This ensures that every call correctly sends cookies for the request url as well as
saving them on response.

This patch was tested using a work in progress implementation of CookieJar which we'll
also look to submit once complete and tested.

Attachments:

1. client.go.patch (2101 bytes)

[vdobler]

Comment 1:

Looks like a real bug.  Client.Post should populate the
request with cookies from its Jar.  My fault.
CL http://golang.org/cl/6013049/ is a bit
less invasive than the supplied patch.

[gopherbot]

Comment 2 by steven.hartland@multiplay.co.uk:

Its less invasive but atm several code paths have duplicate code for no real reason.
This is also fixed by moving all the cookie logic to once central place which produces a
cleaner result.

[bradfitz]

Comment 3:

I think the original patch is fine, but we don't do code reviews or patch submissions
here.  Please use the process:
http://golang.org/doc/contribute.html#Code_review
It will also need tests.
If you'd like me to take this over, let me know, otherwise I'll wait for a new version
of this to be submitted through codereview.

Labels changed: removed priority-triage.

Owner changed to @bradfitz.

Status changed to Accepted.

[gopherbot]

Comment 4 by steven.hartland@multiplay.co.uk:

I can submit it via that process but testing is it a bit of a problem as it requires a
working cookiejar which the current code doesn't have. I have a working implementation
here but its not ready for public consumption yet.
Given this what's the best route to take, maybe extend the current blackhole cookiejar
to simply prints out or stores everything it gets for testing purposes only?

[bradfitz]

Comment 5:

You don't need a "working" cookie jar for a test.  Keep in mind that you're not writing
an end-to-end test here, but rather just a unit test that verifies that the client is
calling the right hooks on the jar at the right times.
What I'd recommend:  in the test file, define your own implementation of CookieJar there:
type fakeJar struct {
   numSets, numGets int
}
func (j *fakeJar) SetCookies(u *url.URL, cookies []*Cookie) {
   j.numSets++
}
func (j *fakeJar) Cookies(u *url.URL) []*Cookie {
   j.numGets++
   return []*Cookie{
       ... "fake-cookie": "fake-value" ...
   }
}
Then, just write a normal http test (using httptest.NewServer, etc) that makes a new
Client using the *fakeJar, and do a Get, Post, Do (HEAD, PUT, etc), and verified that
the httptest Handler gets the fake cookie and/or numSets / numGets increment as
appropriate at the right times.

[vdobler]

Comment 6:

http://golang.org/cl/6013049/
contains the tests (and a sub par fix to avoid breakage).
I suggest reviewing CL 6013049 first, once
the tests are in place Steven can submit
his patch as a CL.

[bradfitz]

Comment 7:

This issue was closed by revision b4456df.

Status changed to Fixed.