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

Issue 116690043: code review 116690043: mime/multipart: add Writer data race test (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 7 months ago by bradfitz
Modified:
10 years, 7 months ago
Reviewers:
ruiu, dvyukov
CC:
ruiu, dvyukov, golang-codereviews, mpl
Visibility:
Public.

Description

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.

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/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+12 lines, -0 lines) Patch
M src/pkg/mime/multipart/writer_test.go View 1 chunk +12 lines, -0 lines 0 comments Download

Messages

Total messages: 9
bradfitz
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/
10 years, 7 months ago (2014-08-05 18:36:29 UTC) #1
dvyukov
LGTM
10 years, 7 months ago (2014-08-05 18:40:48 UTC) #2
ruiu
LGTM
10 years, 7 months ago (2014-08-05 18:43:19 UTC) #3
bradfitz
*** Submitted as https://code.google.com/p/go/source/detail?r=3353b70ecb96 *** mime/multipart: add Writer data race test Camlistore uses this pattern ...
10 years, 7 months ago (2014-08-05 18:45:32 UTC) #4
bradfitz
Actually, I realized after I submitted it: should I wait for the goroutine to complete ...
10 years, 7 months ago (2014-08-05 18:47:14 UTC) #5
dvyukov
On Tue, Aug 5, 2014 at 10:47 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > Actually, I ...
10 years, 7 months ago (2014-08-05 18:58:02 UTC) #6
ruiu
On Tue, Aug 5, 2014 at 11:57 AM, Dmitry Vyukov <dvyukov@google.com> wrote: > On Tue, ...
10 years, 7 months ago (2014-08-05 19:17:00 UTC) #7
dvyukov
On Tue, Aug 5, 2014 at 11:16 PM, Rui Ueyama <ruiu@google.com> wrote: > On Tue, ...
10 years, 7 months ago (2014-08-05 19:22:17 UTC) #8
ruiu
10 years, 7 months ago (2014-08-05 20:20:47 UTC) #9
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.

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