Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(17)

Issue 6497070: code review 6497070: crypto/tls: fix data race on conn.err (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 10 months ago by dfc
Modified:
9 years, 10 months ago
Reviewers:
CC:
dvyukov, agl1, golang-dev
Visibility:
Public.

Description

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.

Patch Set 1 #

Patch Set 2 : diff -r e6da0262840c https://code.google.com/p/go #

Patch Set 3 : diff -r 957c047e5586 https://code.google.com/p/go #

Patch Set 4 : diff -r 957c047e5586 https://code.google.com/p/go #

Total comments: 4

Patch Set 5 : diff -r 957c047e5586 https://code.google.com/p/go #

Patch Set 6 : diff -r 292816148e44 https://code.google.com/p/go #

Patch Set 7 : diff -r f06edb3fcffe https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+32 lines, -31 lines) Patch
M src/pkg/crypto/tls/conn.go View 1 2 3 4 5 8 chunks +30 lines, -29 lines 0 comments Download
M src/pkg/crypto/tls/handshake_client.go View 1 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 7
dfc
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
9 years, 10 months ago (2012-09-05 11:15:37 UTC) #1
dvyukov
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#newcode747 src/pkg/crypto/tls/conn.go:747: if err := c.setError(c.Handshake()); err ...
9 years, 10 months ago (2012-09-05 12:41:39 UTC) #2
agl1
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#newcode47 src/pkg/crypto/tls/conn.go:47: err err is always ...
9 years, 10 months ago (2012-09-05 14:31:30 UTC) #3
dfc
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#newcode47 src/pkg/crypto/tls/conn.go:47: err On 2012/09/05 ...
9 years, 10 months ago (2012-09-06 01:05:00 UTC) #4
dfc
Hello dvyukov@google.com, agl@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
9 years, 10 months ago (2012-09-06 01:05:11 UTC) #5
dvyukov
LGTM
9 years, 10 months ago (2012-09-06 06:36:35 UTC) #6
dfc
9 years, 10 months ago (2012-09-06 07:50:49 UTC) #7
*** 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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b