Issue 116690043: code review 116690043: mime/multipart: add Writer data race test (Closed)
Descriptionmime/multipart: add Writer data race test
Camlistore uses this pattern to do streaming writes, as do
others I imagine, and it was broken by the lazy boundary
change.
Patch Set 1 #Patch Set 2 : diff -r 333b975e98c2 https://go.googlecode.com/hg/ #Patch Set 3 : diff -r 333b975e98c2 https://go.googlecode.com/hg/ #Patch Set 4 : diff -r d664f6385548 https://go.googlecode.com/hg/ #
MessagesTotal messages: 9
Hello ruiu@google.com, dvyukov@google.com (cc: golang-codereviews@googlegroups.com, mathieu.lonjaret@gmail.com), I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
LGTM
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=3353b70ecb96 *** mime/multipart: add Writer data race test Camlistore uses this pattern to do streaming writes, as do others I imagine, and it was broken by the lazy boundary change. LGTM=dvyukov, ruiu R=ruiu, dvyukov CC=golang-codereviews, mathieu.lonjaret https://codereview.appspot.com/116690043
Sign in to reply to this message.
Actually, I realized after I submitted it: should I wait for the goroutine to complete before I let my Test func return? This does the catch the bug for me, but I might be getting lucky with the scheduler? If the goroutine doesn't run for awhile, the Test func could already return? But I guess tsan will continue to run and fail the whole process? Dmitry? On Tue, Aug 5, 2014 at 11:45 AM, <bradfitz@golang.org> wrote: > *** Submitted as > https://code.google.com/p/go/source/detail?r=3353b70ecb96 *** > > > mime/multipart: add Writer data race test > > Camlistore uses this pattern to do streaming writes, as do > others I imagine, and it was broken by the lazy boundary > change. > > LGTM=dvyukov, ruiu > R=ruiu, dvyukov > CC=golang-codereviews, mathieu.lonjaret > https://codereview.appspot.com/116690043 > > > https://codereview.appspot.com/116690043/ >
Sign in to reply to this message.
On Tue, Aug 5, 2014 at 10:47 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > Actually, I realized after I submitted it: should I wait for the goroutine > to complete before I let my Test func return? > > This does the catch the bug for me, but I might be getting lucky with the > scheduler? If the goroutine doesn't run for awhile, the Test func could > already return? > > But I guess tsan will continue to run and fail the whole process? > > Dmitry? Yes, race detector will override exit status of the process regardless of where the race has happened. > On Tue, Aug 5, 2014 at 11:45 AM, <bradfitz@golang.org> wrote: >> >> *** Submitted as >> https://code.google.com/p/go/source/detail?r=3353b70ecb96 *** >> >> >> mime/multipart: add Writer data race test >> >> Camlistore uses this pattern to do streaming writes, as do >> others I imagine, and it was broken by the lazy boundary >> change. >> >> LGTM=dvyukov, ruiu >> R=ruiu, dvyukov >> CC=golang-codereviews, mathieu.lonjaret >> https://codereview.appspot.com/116690043 >> >> >> https://codereview.appspot.com/116690043/ > >
Sign in to reply to this message.
On Tue, Aug 5, 2014 at 11:57 AM, Dmitry Vyukov <dvyukov@google.com> wrote: > On Tue, Aug 5, 2014 at 10:47 PM, Brad Fitzpatrick <bradfitz@golang.org> > wrote: > > Actually, I realized after I submitted it: should I wait for the > goroutine > > to complete before I let my Test func return? > > > > This does the catch the bug for me, but I might be getting lucky with the > > scheduler? If the goroutine doesn't run for awhile, the Test func could > > already return? > > > > But I guess tsan will continue to run and fail the whole process? > > > > Dmitry? > > Yes, race detector will override exit status of the process regardless > of where the race has happened. Does the test wait for all goroutines to complete? If the primordial goroutine exits before the spawned goroutine execute its body, it'll miss the race, no? I think it's a small chance, though. > > > On Tue, Aug 5, 2014 at 11:45 AM, <bradfitz@golang.org> wrote: > >> > >> *** Submitted as > >> https://code.google.com/p/go/source/detail?r=3353b70ecb96 *** > >> > >> > >> mime/multipart: add Writer data race test > >> > >> Camlistore uses this pattern to do streaming writes, as do > >> others I imagine, and it was broken by the lazy boundary > >> change. > >> > >> LGTM=dvyukov, ruiu > >> R=ruiu, dvyukov > >> CC=golang-codereviews, mathieu.lonjaret > >> https://codereview.appspot.com/116690043 > >> > >> > >> https://codereview.appspot.com/116690043/ > > > > >
Sign in to reply to this message.
On Tue, Aug 5, 2014 at 11:16 PM, Rui Ueyama <ruiu@google.com> wrote: > On Tue, Aug 5, 2014 at 11:57 AM, Dmitry Vyukov <dvyukov@google.com> wrote: >> >> On Tue, Aug 5, 2014 at 10:47 PM, Brad Fitzpatrick <bradfitz@golang.org> >> wrote: >> > Actually, I realized after I submitted it: should I wait for the >> > goroutine >> > to complete before I let my Test func return? >> > >> > This does the catch the bug for me, but I might be getting lucky with >> > the >> > scheduler? If the goroutine doesn't run for awhile, the Test func could >> > already return? >> > >> > But I guess tsan will continue to run and fail the whole process? >> > >> > Dmitry? >> >> Yes, race detector will override exit status of the process regardless >> of where the race has happened. > > > Does the test wait for all goroutines to complete? no > If the primordial > goroutine exits before the spawned goroutine execute its body, it'll miss > the race, no? I think it's a small chance, though. We won't miss the race if the test goroutine exits. But we will miss the race if the process exits before the second goroutine executes. >> > On Tue, Aug 5, 2014 at 11:45 AM, <bradfitz@golang.org> wrote: >> >> >> >> *** Submitted as >> >> https://code.google.com/p/go/source/detail?r=3353b70ecb96 *** >> >> >> >> >> >> mime/multipart: add Writer data race test >> >> >> >> Camlistore uses this pattern to do streaming writes, as do >> >> others I imagine, and it was broken by the lazy boundary >> >> change. >> >> >> >> LGTM=dvyukov, ruiu >> >> R=ruiu, dvyukov >> >> CC=golang-codereviews, mathieu.lonjaret >> >> https://codereview.appspot.com/116690043 >> >> >> >> >> >> https://codereview.appspot.com/116690043/ >> > >> > > >
Sign in to reply to this message.
On Tue, Aug 5, 2014 at 12:21 PM, Dmitry Vyukov <dvyukov@google.com> wrote: > On Tue, Aug 5, 2014 at 11:16 PM, Rui Ueyama <ruiu@google.com> wrote: > > On Tue, Aug 5, 2014 at 11:57 AM, Dmitry Vyukov <dvyukov@google.com> > wrote: > >> > >> On Tue, Aug 5, 2014 at 10:47 PM, Brad Fitzpatrick <bradfitz@golang.org> > >> wrote: > >> > Actually, I realized after I submitted it: should I wait for the > >> > goroutine > >> > to complete before I let my Test func return? > >> > > >> > This does the catch the bug for me, but I might be getting lucky with > >> > the > >> > scheduler? If the goroutine doesn't run for awhile, the Test func > could > >> > already return? > >> > > >> > But I guess tsan will continue to run and fail the whole process? > >> > > >> > Dmitry? > >> > >> Yes, race detector will override exit status of the process regardless > >> of where the race has happened. > > > > > > Does the test wait for all goroutines to complete? > > no > > > If the primordial > > goroutine exits before the spawned goroutine execute its body, it'll miss > > the race, no? I think it's a small chance, though. > > We won't miss the race if the test goroutine exits. But we will miss > the race if the process exits before the second goroutine executes. Then we probably should wait for the goroutine. It's an easy thing to do. > > > >> > On Tue, Aug 5, 2014 at 11:45 AM, <bradfitz@golang.org> wrote: > >> >> > >> >> *** Submitted as > >> >> https://code.google.com/p/go/source/detail?r=3353b70ecb96 *** > >> >> > >> >> > >> >> mime/multipart: add Writer data race test > >> >> > >> >> Camlistore uses this pattern to do streaming writes, as do > >> >> others I imagine, and it was broken by the lazy boundary > >> >> change. > >> >> > >> >> LGTM=dvyukov, ruiu > >> >> R=ruiu, dvyukov > >> >> CC=golang-codereviews, mathieu.lonjaret > >> >> https://codereview.appspot.com/116690043 > >> >> > >> >> > >> >> https://codereview.appspot.com/116690043/ > >> > > >> > > > > > >
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||
