Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(242)
Issues Repositories Search
Open Issues | Closed Issues | All Issues | Sign in with your Google Account to create issues and add comments

Issue 95760043: code review 95760043: mime/multipart: delay reading random source (Closed)

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

Description

mime/multipart: delay reading random source If a user sets his/her own boundary string with SetBoundary, we don't need to call randomBoundary at all.

Patch Set 1 #

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

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

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+8 lines, -6 lines) Patch
M src/pkg/mime/multipart/writer.go View 4 chunks +8 lines, -6 lines 0 comments Download

Messages

Total messages: 6
ruiu
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
11 years, 5 months ago (2014-06-11 03:13:50 UTC) #1
bradfitz
LGTM
11 years, 5 months ago (2014-06-11 18:32:14 UTC) #2
ruiu
*** Submitted as https://code.google.com/p/go/source/detail?r=b2131d729e52 *** mime/multipart: delay reading random source If a user sets his/her ...
11 years, 3 months ago (2014-07-30 01:24:52 UTC) #3
bradfitz
This broke Camlistore. We didn't believe it at first and thought our continuous build (which ...
11 years, 3 months ago (2014-08-05 18:12:50 UTC) #4
bradfitz
I see. It's a data race. Camlistore does: pipeReader, pipeWriter := io.Pipe() multipartWriter := *multipart.NewWriter*(pipeWriter) ...
11 years, 3 months ago (2014-08-05 18:24:04 UTC) #5
dvyukov
11 years, 3 months ago (2014-08-05 18:29:46 UTC) #6
Message was sent while issue was closed. 
On 2014/08/05 18:24:04, bradfitz wrote:
> I see. It's a data race.
> 
> Camlistore does:
> 
>         pipeReader, pipeWriter := io.Pipe()
>         multipartWriter := *multipart.NewWriter*(pipeWriter)
> 
>         copyResult := make(chan error, 1)
> go func() {
>                 defer pipeWriter.Close()
>                 part, err := *multipartWriter.CreateFormFile*(blobrefStr,
> blobrefStr)
>         if err != nil {
>                 copyResult <- err
>                 return
>         }
>         _, err = io.Copy(part, bodyReader)
>         if err == nil {
>                 err = multipartWriter.Close()
>         }
>         copyResult <- err
> }()
> 
> uploadURL := fmt.Sprintf("%s/camli/upload", pfx)
>         req := c.newRequest("POST", uploadURL)
>       req.Header.Set("Content-Type", *multipartWriter.FormDataContentType*
> ())
> 
> ==================
> WARNING: DATA RACE
> Read by goroutine 32:
>   mime/multipart.(*Writer).Boundary()
>       /Users/bradfitz/go/src/pkg/mime/multipart/writer.go:34 +0x4c
>   mime/multipart.(*Writer).CreatePart()
>       /Users/bradfitz/go/src/pkg/mime/multipart/writer.go:96 +0x975
>   mime/multipart.(*Writer).CreateFormFile()
>       /Users/bradfitz/go/src/pkg/mime/multipart/writer.go:131 +0x27e
>   camlistore.org/pkg/client.func·013()
> <http://camlistore.org/pkg/client.func%C2%B7013()>
>       /Users/bradfitz/src/
>
camlistore.org/tmp/build-gopath-nosqlite/src/camlistore.org/pkg/client/upload.go:436
> +0xe6
> 
> Previous write by goroutine 31:
>   mime/multipart.(*Writer).Boundary()
>       /Users/bradfitz/go/src/pkg/mime/multipart/writer.go:35 +0x86
>   mime/multipart.(*Writer).FormDataContentType()
>       /Users/bradfitz/go/src/pkg/mime/multipart/writer.go:70 +0x46
>   camlistore.org/pkg/client.(*Client).Upload()
>       /Users/bradfitz/src/
>
camlistore.org/tmp/build-gopath-nosqlite/src/camlistore.org/pkg/client/upload.go:453
> +0x1ac5
>   camlistore.org/pkg/client.(*Client).ReceiveBlob()
>       /Users/bradfitz/src/
>
camlistore.org/tmp/build-gopath-nosqlite/src/camlistore.org/pkg/client/get.go:177
> +0x2bc
>   main.(*noStatReceiver).ReceiveBlob()
>       <autogenerated>:166 +0xd1
>   camlistore.org/pkg/blobserver.receive()
>       /Users/bradfitz/src/
>
camlistore.org/tmp/build-gopath-nosqlite/src/camlistore.org/pkg/blobserver/receive.go:50
> +0x2b8
>   camlistore.org/pkg/blobserver.ReceiveNoHash()
>       /Users/bradfitz/src/
>
camlistore.org/tmp/build-gopath-nosqlite/src/camlistore.org/pkg/blobserver/receive.go:42
> +0xaf
>   camlistore.org/pkg/schema.uploadString()
>       /Users/bradfitz/src/
>
camlistore.org/tmp/build-gopath-nosqlite/src/camlistore.org/pkg/schema/filewriter.go:197
> +0x30e
>   camlistore.org/pkg/schema.func·008()
> <http://camlistore.org/pkg/schema.func%C2%B7008()>
>       /Users/bradfitz/src/
>
camlistore.org/tmp/build-gopath-nosqlite/src/camlistore.org/pkg/schema/filewriter.go:367
> +0xf5
> 
> Goroutine 32 (running) created at:
>   camlistore.org/pkg/client.(*Client).Upload()
>       /Users/bradfitz/src/
>
camlistore.org/tmp/build-gopath-nosqlite/src/camlistore.org/pkg/client/upload.go:446
> +0x1958
>   camlistore.org/pkg/client.(*Client).ReceiveBlob()
>       /Users/bradfitz/src/
>
camlistore.org/tmp/build-gopath-nosqlite/src/camlistore.org/pkg/client/get.go:177
> +0x2bc
>   main.(*noStatReceiver).ReceiveBlob()
>       <autogenerated>:166 +0xd1
>   camlistore.org/pkg/blobserver.receive()
>       /Users/bradfitz/src/
>
camlistore.org/tmp/build-gopath-nosqlite/src/camlistore.org/pkg/blobserver/receive.go:50
> +0x2b8
>   camlistore.org/pkg/blobserver.ReceiveNoHash()
>       /Users/bradfitz/src/
>
camlistore.org/tmp/build-gopath-nosqlite/src/camlistore.org/pkg/blobserver/receive.go:42
> +0xaf
>   camlistore.org/pkg/schema.uploadString()
>       /Users/bradfitz/src/
>
camlistore.org/tmp/build-gopath-nosqlite/src/camlistore.org/pkg/schema/filewriter.go:197
> +0x30e
>   camlistore.org/pkg/schema.func·008()
> <http://camlistore.org/pkg/schema.func%C2%B7008()>
>       /Users/bradfitz/src/
>
camlistore.org/tmp/build-gopath-nosqlite/src/camlistore.org/pkg/schema/filewriter.go:367
> +0xf5
> 
> Goroutine 31 (running) created at:
>   camlistore.org/pkg/schema.func·009()
> <http://camlistore.org/pkg/schema.func%C2%B7009()>
>       /Users/bradfitz/src/
>
camlistore.org/tmp/build-gopath-nosqlite/src/camlistore.org/pkg/schema/filewriter.go:373
> +0x437
>   camlistore.org/pkg/schema.writeFileChunks()
>       /Users/bradfitz/src/
>
camlistore.org/tmp/build-gopath-nosqlite/src/camlistore.org/pkg/schema/filewriter.go:382
> +0x6ca
>   camlistore.org/pkg/schema.writeFileMapRolling()
>       /Users/bradfitz/src/
>
camlistore.org/tmp/build-gopath-nosqlite/src/camlistore.org/pkg/schema/filewriter.go:312
> +0xa1
>   camlistore.org/pkg/schema.WriteFileMap()
>       /Users/bradfitz/src/
>
camlistore.org/tmp/build-gopath-nosqlite/src/camlistore.org/pkg/schema/filewriter.go:78
> +0x90
>   main.(*Uploader).uploadNodeRegularFile()
>       /Users/bradfitz/src/
>
camlistore.org/tmp/build-gopath-nosqlite/src/camlistore.org/cmd/camput/files.go:631
> +0x13ad
>   main.(*Uploader).uploadNode()
>       /Users/bradfitz/src/
>
camlistore.org/tmp/build-gopath-nosqlite/src/camlistore.org/cmd/camput/files.go:381
> +0xc7
>   main.func·018()
>       /Users/bradfitz/src/
>
camlistore.org/tmp/build-gopath-nosqlite/src/camlistore.org/cmd/camput/files.go:1040
> +0x1a9
>   main.(*nodeWorker).work()
>       /Users/bradfitz/src/
>
camlistore.org/tmp/build-gopath-nosqlite/src/camlistore.org/cmd/camput/chanworker.go:116
> +0x115
> ==================
> 
> 
> On Tue, Aug 5, 2014 at 11:12 AM, Brad Fitzpatrick <mailto:bradfitz@golang.org>
> wrote:
> 
> > This broke Camlistore.
> >
> > We didn't believe it at first and thought our continuous build (which
> > tests both Go release & Go tip) was broken, but two of us have now
> > independently confirmed that tests pass immediately before this change and
> > reliably fail with this change.
> >
> > The server reports errors like:
> > 2014/08/05 11:05:58 Client error: Error reading multipart section:
> > multipart: NextPart: EOF
> >
> > I haven't identified exactly why yet, though. The change still looks safe
> > to my eyes.
> >
> >
> >
> >
> > On Tue, Jul 29, 2014 at 6:24 PM, <mailto:ruiu@google.com> wrote:
> >
> >> *** Submitted as
> >> https://code.google.com/p/go/source/detail?r=b2131d729e52 ***
> >>
> >> mime/multipart: delay reading random source
> >>
> >> If a user sets his/her own boundary string with SetBoundary,
> >> we don't need to call randomBoundary at all.
> >>
> >> LGTM=bradfitz
> >> R=golang-codereviews, bradfitz
> >> CC=golang-codereviews
> >> https://codereview.appspot.com/95760043
> >>
> >>
> >> https://codereview.appspot.com/95760043/
> >>
> >
> >

Can we have this as a test in std lib? We have so few good concurrent tests...
Sign in to reply to this message.

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