code review 6497070: crypto/tls: fix data race on conn.err

crypto/tls: fix data race on conn.err

Fixes issue 3862.

There were many areas where conn.err was being accessed
outside the mutex. This proposal moves the err value to
an embedded struct to make it more obvious when the error
value is being accessed.

As there are no Benchmark tests in this package I cannot
feel confident of the impact of this additional locking,
although most will be uncontended.

[dfc, 2012-09-05 11:15:37]

Hello dvyukov@google.com, agl@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[dvyukov, 2012-09-05 12:41:39]

LGTM on my side

https://codereview.appspot.com/6497070/diff/6001/src/pkg/crypto/tls/conn.go
File src/pkg/crypto/tls/conn.go (right):

https://codereview.appspot.com/6497070/diff/6001/src/pkg/crypto/tls/conn.go#n...
src/pkg/crypto/tls/conn.go:747: if err := c.setError(c.Handshake()); err != nil
{
Somewhat obscure on my taste. I would write something along the lines of:
if err := c.Handshake; err != nil {
  return 0, c.setError(err)
}
Your current use also requires err != nil check in setError(), otherwise you
will lock/unlock/defer on every Write.

[agl1, 2012-09-05 14:31:30]

LGTM with dvyukov's points addressed.

https://codereview.appspot.com/6497070/diff/6001/src/pkg/crypto/tls/conn.go
File src/pkg/crypto/tls/conn.go (right):

https://codereview.appspot.com/6497070/diff/6001/src/pkg/crypto/tls/conn.go#n...
src/pkg/crypto/tls/conn.go:47: err
err is always a variable name for an error and using it as a struct name tripped
me up.

connErr?

[dfc, 2012-09-06 01:05:00]

Thank you for your comments. PTAL.

https://codereview.appspot.com/6497070/diff/6001/src/pkg/crypto/tls/conn.go
File src/pkg/crypto/tls/conn.go (right):

https://codereview.appspot.com/6497070/diff/6001/src/pkg/crypto/tls/conn.go#n...
src/pkg/crypto/tls/conn.go:47: err
On 2012/09/05 14:31:30, agl1 wrote:
> err is always a variable name for an error and using it as a struct name
tripped
> me up.
> 
> connErr?

STGM. PTAL.

https://codereview.appspot.com/6497070/diff/6001/src/pkg/crypto/tls/conn.go#n...
src/pkg/crypto/tls/conn.go:747: if err := c.setError(c.Handshake()); err != nil
{
On 2012/09/05 12:41:39, dvyukov wrote:
> Somewhat obscure on my taste. I would write something along the lines of:
> if err := c.Handshake; err != nil {
>   return 0, c.setError(err)
> }
> Your current use also requires err != nil check in setError(), otherwise you
> will lock/unlock/defer on every Write.

Done.

[dfc, 2012-09-06 01:05:11]

Hello dvyukov@google.com, agl@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[dvyukov, 2012-09-06 06:36:35]

LGTM

[dfc, 2012-09-06 07:50:49]

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

crypto/tls: fix data race on conn.err

Fixes issue 3862.

There were many areas where conn.err was being accessed
outside the mutex. This proposal moves the err value to
an embedded struct to make it more obvious when the error
value is being accessed.

As there are no Benchmark tests in this package I cannot
feel confident of the impact of this additional locking,
although most will be uncontended.

R=dvyukov, agl
CC=golang-dev
http://codereview.appspot.com/6497070