|
|
|
Created:
11 years, 11 months ago by paul.querna Modified:
11 years, 10 months ago CC:
golang-codereviews, r, rsc Visibility:
Public. |
net/http: Add TLS Connection State to Responses.
Fixes issue 7289.
Patch Set 1 #Patch Set 2 : diff -r ec5195954667 https://code.google.com/p/go #Patch Set 3 : diff -r ec5195954667 https://code.google.com/p/go #
Total comments: 3
Total messages: 15
|
paul.querna
Hello golang-codereviews@googlegroups.com, golang-dev@googlegroups.com (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
|
11 years, 11 months ago (2014年02月13日 06:29:48 UTC) #1 | |||||||||||||||||||||||||||||||
Hello golang-codereviews@googlegroups.com, golang-dev@googlegroups.com (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
Replacing golang-dev with golang-codereviews. To the author of this CL: If you are using 'hg mail -r golang-dev' to mail the CL, use simply 'hg mail' instead. If you did not name golang-dev explicitly and it was still added to the CL, it means your working copy of the repo has a stale codereview.cfg (or lib/codereview/codereview.cfg). Please run 'hg sync' to update your client to the most recent codereview.cfg. If the most recent codereview.cfg has defaultcc set to golang-dev instead of golang-codereviews, please send a CL correcting it. Thanks very much.
minor points. Leaving for agl and bradfitz for semantics and security. https://codereview.appspot.com/52660047/diff/40001/src/pkg/net/http/client_te... File src/pkg/net/http/client_test.go (right): https://codereview.appspot.com/52660047/diff/40001/src/pkg/net/http/client_te... src/pkg/net/http/client_test.go:703: t.Errorf("Unexpected TLS Cipher Suite: %d != %d", %d != %d doesn't tell us which is expected and which actually happened. rephrase. https://codereview.appspot.com/52660047/diff/40001/src/pkg/net/http/response.go File src/pkg/net/http/response.go (right): https://codereview.appspot.com/52660047/diff/40001/src/pkg/net/http/response.... src/pkg/net/http/response.go:79: // TLS allows information about the TLS connection on which the s/allows/contains/ https://codereview.appspot.com/52660047/diff/40001/src/pkg/net/http/response.... src/pkg/net/http/response.go:82: // it leaves the field nil. If the connection uses TLS, this field is set by the Transport before returning the Response.
ping agl, bradfitz
I think this is up to bradfitz. My only comment would be that "TLS" is a bit ambiguous of a member name.
It's the same name and convention as Request.TLS, though: http://golang.org/pkg/net/http/#Request The code & tests seem fine, and I'm happy with the name. My one concern would be that it's 96 new bytes of garbage per request when most people will never look at this. (and tls.ConnectionState has been growing... a field in Go 1.1 and again in Go 1.3) I think I would prefer an accessor to get at the TLS state, but I wouldn't be surprised if others here would prefer consistency and to trust the GC to do a good job. But I already think the net/http package generates too much garbage and I'd like to stop the bleeding with new CLs, rather than just accept it. Russ, want to make the call? Field, accessor, don't care? On Mon, Mar 3, 2014 at 5:23 PM, <agl@golang.org> wrote: > I think this is up to bradfitz. > > My only comment would be that "TLS" is a bit ambiguous of a member name. > > https://codereview.appspot.com/52660047/ >
The tricky bit about field vs accessor is that things other than package http need to be able to create/fill out a Response, so whatever the accessor is working from needs to be an exported field itself. One option is to make the field have type ConnectionState instead of *ConnectionState, but I assume the size increase here is unwanted. Perhaps it would suffice to use a field of type *ConnectionState but only bother to allocate one such pointer per connection, so that a persistent HTTPS connection will use one for the whole connection, not one per response received. There's already other per-connection TLS garbage so maybe it's more in the noise that way. I agree that we might want to make Response a bit more opaque in Go 2.
Re-usable pointer could work. Most connections are keep-alive anyway, and
if not: you have bigger performances problems than 96 bytes.
Also for consideration, alternatively or additionally: flag on Client or
Transport (and maybe Server later) to say whether you do/don't care about
{Request,Response}.TLS. We use the flag convention at least in the log
package when making a logger. Thoughts?
On Mon, Mar 3, 2014 at 5:41 PM, <rsc@golang.org> wrote:
> The tricky bit about field vs accessor is that things other than package
> http need to be able to create/fill out a Response, so whatever the
> accessor is working from needs to be an exported field itself.
>
> One option is to make the field have type ConnectionState instead of
> *ConnectionState, but I assume the size increase here is unwanted.
>
> Perhaps it would suffice to use a field of type *ConnectionState but
> only bother to allocate one such pointer per connection, so that a
> persistent HTTPS connection will use one for the whole connection, not
> one per response received. There's already other per-connection TLS
> garbage so maybe it's more in the noise that way.
>
> I agree that we might want to make Response a bit more opaque in Go 2.
>
> https://codereview.appspot.com/52660047/
>
let's just do the single field + reuse for now. we can worry about flags and such when the time comes to rethink everything, but for now let's just be consistent with what's there.
SGTM Paul, does that all make sense? On Mon, Mar 3, 2014 at 5:58 PM, Russ Cox <rsc@golang.org> wrote: > let's just do the single field + reuse for now. we can worry about flags > and such when the time comes to rethink everything, but for now let's just > be consistent with what's there. > >
paul, will you have time to work on this in the next few days? if not, brad, can you create a CL instead?
LGTM I will submit this and then fix a couple things up (including the memory optimization) in a follow-up CL.
*** Submitted as https://code.google.com/p/go/source/detail?r=4f40188a1999 *** net/http: Add TLS Connection State to Responses. Fixes issue 7289. LGTM=bradfitz R=golang-codereviews, r, bradfitz, rsc CC=golang-codereviews https://codereview.appspot.com/52660047 Committer: Brad Fitzpatrick <bradfitz@golang.org>
Follow-ups in https://codereview.appspot.com/71740043