code review 52660047: net/http: Add TLS Connection State to Responses.

net/http: Add TLS Connection State to Responses.

Fixes issue 7289.

[paul.querna, 2014-02-13 06:29:48]

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

[gobot, 2014-02-13 06:33:51]

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.

[r, 2014-02-13 19:41:13]

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.

[rsc, 2014-03-03 23:48:59]

ping agl, bradfitz

[agl1, 2014-03-04 01:23:40]

I think this is up to bradfitz.

My only comment would be that "TLS" is a bit ambiguous of a member name.

[bradfitz, 2014-03-04 01:35:54]

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/
>

[rsc, 2014-03-04 01:41:52]

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.

[bradfitz, 2014-03-04 01:51:44]

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/
>

[rsc, 2014-03-04 01:58:35]

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.

[bradfitz, 2014-03-04 02:00:19]

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.
>
>

[rsc, 2014-03-05 19:59:35]

paul, will you have time to work on this in the next few days?
if not, brad, can you create a CL instead?

[bradfitz, 2014-03-05 20:25:41]

LGTM

I will submit this and then fix a couple things up (including the memory
optimization) in a follow-up CL.

[bradfitz, 2014-03-05 20:25:57]

*** 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>

[bradfitz, 2014-03-05 20:35:59]

Follow-ups in https://codereview.appspot.com/71740043

[gobot, 2014-03-05 22:56:00]

This CL appears to have broken the windows-386-ec2 builder.