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

Issue 6944057: code review 6944057: net/smtp: remove data race from TestSendMail. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 6 months ago by rick
Modified:
9 years, 6 months ago
Reviewers:
CC:
golang-dev, bradfitz, dfc, rsc
Visibility:
Public.

Description

net/smtp: remove data race from TestSendMail. A data race was found in TestSendMail by the race detector. Fixes issue 4559.

Patch Set 1 #

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

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

Total comments: 8

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

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

Total comments: 3

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

Patch Set 7 : diff -r 5083a8d8cc89 https://code.google.com/p/go #

Total comments: 4

Patch Set 8 : diff -r 5083a8d8cc89 https://code.google.com/p/go #

Unified diffs Side-by-side diffs Delta from patch set Stats (+30 lines, -23 lines) Patch
M src/pkg/net/smtp/smtp_test.go View 1 2 3 4 5 6 7 9 chunks +30 lines, -23 lines 0 comments Download

Messages

Total messages: 18
rick
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
9 years, 6 months ago (2012-12-17 04:46:18 UTC) #1
bradfitz
https://codereview.appspot.com/6944057/diff/4001/src/pkg/net/smtp/smtp_test.go File src/pkg/net/smtp/smtp_test.go (right): https://codereview.appspot.com/6944057/diff/4001/src/pkg/net/smtp/smtp_test.go#newcode389 src/pkg/net/smtp/smtp_test.go:389: var mu sync.Mutex what is this guarding? it's not ...
9 years, 6 months ago (2012-12-17 04:48:20 UTC) #2
dfc
https://codereview.appspot.com/6944057/diff/4001/src/pkg/net/smtp/smtp_test.go File src/pkg/net/smtp/smtp_test.go (right): https://codereview.appspot.com/6944057/diff/4001/src/pkg/net/smtp/smtp_test.go#newcode389 src/pkg/net/smtp/smtp_test.go:389: var mu sync.Mutex you can also use a channel ...
9 years, 6 months ago (2012-12-17 04:49:29 UTC) #3
rick
PTAL Thanks guys. https://codereview.appspot.com/6944057/diff/4001/src/pkg/net/smtp/smtp_test.go File src/pkg/net/smtp/smtp_test.go (right): https://codereview.appspot.com/6944057/diff/4001/src/pkg/net/smtp/smtp_test.go#newcode389 src/pkg/net/smtp/smtp_test.go:389: var mu sync.Mutex On 2012/12/17 04:48:20, ...
9 years, 6 months ago (2012-12-17 04:57:14 UTC) #4
dfc
I can confirm the race detector no longer complains on my host. https://codereview.appspot.com/6944057/diff/8003/src/pkg/net/smtp/smtp_test.go File src/pkg/net/smtp/smtp_test.go ...
9 years, 6 months ago (2012-12-17 05:02:47 UTC) #5
rick
https://codereview.appspot.com/6944057/diff/8003/src/pkg/net/smtp/smtp_test.go File src/pkg/net/smtp/smtp_test.go (right): https://codereview.appspot.com/6944057/diff/8003/src/pkg/net/smtp/smtp_test.go#newcode397 src/pkg/net/smtp/smtp_test.go:397: t.Log("Accept error: %v", err) On 2012/12/17 05:02:47, dfc wrote: ...
9 years, 6 months ago (2012-12-17 05:05:56 UTC) #6
dfc
> I am counting on the later Got / Expected test to log the error. ...
9 years, 6 months ago (2012-12-17 05:07:02 UTC) #7
rick
PTAL
9 years, 6 months ago (2012-12-17 05:09:45 UTC) #8
dfc
Please also check that the test passes with go test -cpu=1,2,4 On Mon, Dec 17, ...
9 years, 6 months ago (2012-12-17 05:22:13 UTC) #9
rick
On 2012/12/17 05:22:13, dfc wrote: > Please also check that the test passes with > ...
9 years, 6 months ago (2012-12-17 05:27:19 UTC) #10
dfc
If you can, otherwise fullung will raise more issues :) On Mon, Dec 17, 2012 ...
9 years, 6 months ago (2012-12-17 05:35:47 UTC) #11
rick
PTAL The original tests were modifying global vars instead of making local copies.
9 years, 6 months ago (2012-12-17 05:42:57 UTC) #12
dfc
On 2012/12/17 05:42:57, rick wrote: > PTAL > > The original tests were modifying global ...
9 years, 6 months ago (2012-12-17 05:46:45 UTC) #13
rick
> Thanks Rick, sorry this turned into such a drama. > No problem; I learned ...
9 years, 6 months ago (2012-12-17 05:50:14 UTC) #14
dfc
LGTM with one final comment. https://codereview.appspot.com/6944057/diff/4002/src/pkg/net/smtp/smtp_test.go File src/pkg/net/smtp/smtp_test.go (right): https://codereview.appspot.com/6944057/diff/4002/src/pkg/net/smtp/smtp_test.go#newcode394 src/pkg/net/smtp/smtp_test.go:394: i := 0 please ...
9 years, 6 months ago (2012-12-17 06:56:50 UTC) #15
rick
PTAL https://codereview.appspot.com/6944057/diff/4002/src/pkg/net/smtp/smtp_test.go File src/pkg/net/smtp/smtp_test.go (right): https://codereview.appspot.com/6944057/diff/4002/src/pkg/net/smtp/smtp_test.go#newcode394 src/pkg/net/smtp/smtp_test.go:394: i := 0 On 2012/12/17 06:56:50, dfc wrote: ...
9 years, 6 months ago (2012-12-17 12:54:46 UTC) #16
rsc
LGTM
9 years, 6 months ago (2012-12-17 15:45:07 UTC) #17
rsc
9 years, 6 months ago (2012-12-17 15:45:36 UTC) #18
*** Submitted as https://code.google.com/p/go/source/detail?r=9530d8bc74cb ***

net/smtp: remove data race from TestSendMail.

A data race was found in TestSendMail by the race detector.

Fixes issue 4559.

R=golang-dev, bradfitz, dave, rsc
CC=golang-dev
https://codereview.appspot.com/6944057

Committer: Russ Cox <rsc@golang.org>
Sign in to reply to this message.

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