Issue 6944057: code review 6944057: net/smtp: remove data race from TestSendMail. (Closed)
|
Created:
11 years, 4 months ago by rick Modified:
11 years, 4 months ago Reviewers:
CC:
golang-dev, bradfitz, dfc, rsc Visibility:
Public. |
Descriptionnet/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 #MessagesTotal messages: 18
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
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.g... src/pkg/net/smtp/smtp_test.go:389: var mu sync.Mutex what is this guarding? it's not obvious. add a comment? https://codereview.appspot.com/6944057/diff/4001/src/pkg/net/smtp/smtp_test.g... src/pkg/net/smtp/smtp_test.go:426: }(l, strings.Split(server, "\r\n"), bcmdbuf, &mu) no need to pass this to the goroutine. the goroutine is a closure... it closes over the outer "mu".
Sign in to reply to this message.
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.g... src/pkg/net/smtp/smtp_test.go:389: var mu sync.Mutex you can also use a channel to synchronise here var done = make(chan struct{}) then defer(close) inside the function. https://codereview.appspot.com/6944057/diff/4001/src/pkg/net/smtp/smtp_test.g... src/pkg/net/smtp/smtp_test.go:390: go func(l net.Listener, data []string, w *bufio.Writer, mu *sync.Mutex) { you don't need to pass w and mu into the function, the closure already captures them.
Sign in to reply to this message.
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.g... src/pkg/net/smtp/smtp_test.go:389: var mu sync.Mutex On 2012/12/17 04:48:20, bradfitz wrote: > what is this guarding? it's not obvious. add a comment? Done https://codereview.appspot.com/6944057/diff/4001/src/pkg/net/smtp/smtp_test.g... src/pkg/net/smtp/smtp_test.go:389: var mu sync.Mutex On 2012/12/17 04:49:29, dfc wrote: > you can also use a channel to synchronise here > > var done = make(chan struct{}) > > then defer(close) inside the function. Done. https://codereview.appspot.com/6944057/diff/4001/src/pkg/net/smtp/smtp_test.g... src/pkg/net/smtp/smtp_test.go:390: go func(l net.Listener, data []string, w *bufio.Writer, mu *sync.Mutex) { On 2012/12/17 04:49:29, dfc wrote: > you don't need to pass w and mu into the function, the closure already captures > them. Done. Also removed l. https://codereview.appspot.com/6944057/diff/4001/src/pkg/net/smtp/smtp_test.g... src/pkg/net/smtp/smtp_test.go:426: }(l, strings.Split(server, "\r\n"), bcmdbuf, &mu) On 2012/12/17 04:48:20, bradfitz wrote: > no need to pass this to the goroutine. the goroutine is a closure... it closes > over the outer "mu". > Done
Sign in to reply to this message.
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 (right): https://codereview.appspot.com/6944057/diff/8003/src/pkg/net/smtp/smtp_test.g... src/pkg/net/smtp/smtp_test.go:397: t.Log("Accept error: %v", err) Should this be t.Errorf ? https://codereview.appspot.com/6944057/diff/8003/src/pkg/net/smtp/smtp_test.g... src/pkg/net/smtp/smtp_test.go:418: t.Log("Read error: %v", err) ditto
Sign in to reply to this message.
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.g... src/pkg/net/smtp/smtp_test.go:397: t.Log("Accept error: %v", err) On 2012/12/17 05:02:47, dfc wrote: > Should this be t.Errorf ? I am counting on the later Got / Expected test to log the error. Both of the Logged errors would cause that test to fail. Is it ok to log multiple errors from a single test?
Sign in to reply to this message.
> I am counting on the later Got / Expected test to log the error. Both of the > Logged errors would cause that test to fail. Is it ok to log multiple errors > from a single test? yup, in fact you cannot use t.Fatalf in a goroutine.
Sign in to reply to this message.
PTAL
Sign in to reply to this message.
Please also check that the test passes with go test -cpu=1,2,4 On Mon, Dec 17, 2012 at 4:09 PM, <rickarnoldjr@gmail.com> wrote: > PTAL > > > https://codereview.appspot.com/6944057/
Sign in to reply to this message.
On 2012/12/17 05:22:13, dfc wrote: > Please also check that the test passes with > > go test -cpu=1,2,4 > Hmm. Well, looks like TestSendMail passes, but the others all fail. Do you want me to fix that as part of this CL? --- FAIL: TestBasic-2 (0.00 seconds) smtp_test.go:93: Expected AUTH supported --- FAIL: TestNewClient-2 (0.00 seconds) smtp_test.go:207: Expected AUTH supported --- FAIL: TestNewClient2-2 (0.00 seconds) smtp_test.go:256: Got: EHLO localhost HELO localhost QUIT Expected: EHLO localhost HELO localhost QUIT --- FAIL: TestBasic-4 (0.00 seconds) smtp_test.go:93: Expected AUTH supported --- FAIL: TestNewClient-4 (0.00 seconds) smtp_test.go:207: Expected AUTH supported --- FAIL: TestNewClient2-4 (0.00 seconds) smtp_test.go:256: Got: EHLO localhost HELO localhost QUIT Expected: EHLO localhost HELO localhost QUIT FAIL exit status 1 FAIL net/smtp 0.068s
Sign in to reply to this message.
If you can, otherwise fullung will raise more issues :) On Mon, Dec 17, 2012 at 4:27 PM, <rickarnoldjr@gmail.com> wrote: > On 2012/12/17 05:22:13, dfc wrote: >> >> Please also check that the test passes with > > >> go test -cpu=1,2,4 > > > > Hmm. Well, looks like TestSendMail passes, but the others all fail. Do > you want me to fix that as part of this CL? > > --- FAIL: TestBasic-2 (0.00 seconds) > smtp_test.go:93: Expected AUTH supported > --- FAIL: TestNewClient-2 (0.00 seconds) > smtp_test.go:207: Expected AUTH supported > --- FAIL: TestNewClient2-2 (0.00 seconds) > smtp_test.go:256: Got: > EHLO localhost > HELO localhost > QUIT > > Expected: > EHLO localhost > HELO localhost > QUIT > --- FAIL: TestBasic-4 (0.00 seconds) > smtp_test.go:93: Expected AUTH supported > --- FAIL: TestNewClient-4 (0.00 seconds) > smtp_test.go:207: Expected AUTH supported > --- FAIL: TestNewClient2-4 (0.00 seconds) > smtp_test.go:256: Got: > EHLO localhost > HELO localhost > QUIT > > Expected: > EHLO localhost > HELO localhost > QUIT > FAIL > exit status 1 > FAIL net/smtp 0.068s > > > > https://codereview.appspot.com/6944057/
Sign in to reply to this message.
PTAL The original tests were modifying global vars instead of making local copies.
Sign in to reply to this message.
On 2012/12/17 05:42:57, rick wrote: > PTAL > > The original tests were modifying global vars instead of making local copies. Thanks Rick, sorry this turned into such a drama. Leaving for bradfitz.
Sign in to reply to this message.
> Thanks Rick, sorry this turned into such a drama. > No problem; I learned a lot from this. :) Rick
Sign in to reply to this message.
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.g... src/pkg/net/smtp/smtp_test.go:394: i := 0 please move this into the for loop https://codereview.appspot.com/6944057/diff/4002/src/pkg/net/smtp/smtp_test.g... src/pkg/net/smtp/smtp_test.go:403: for i < len(data) && data[i] != "" { for i := 0 ; i < ... ; i++ { https://codereview.appspot.com/6944057/diff/4002/src/pkg/net/smtp/smtp_test.g... src/pkg/net/smtp/smtp_test.go:425: i++ drop
Sign in to reply to this message.
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.g... src/pkg/net/smtp/smtp_test.go:394: i := 0 On 2012/12/17 06:56:50, dfc wrote: > please move this into the for loop Done. Not sure how I ended up with this. :)
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** 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.
|
