Issue 77580046: code review 77580046: strings: fix Reader.UnreadRune

ruiu

Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go

11 years, 9 months ago (2014-03-19 06:31:36 UTC) #1

bradfitz

Is bytes.Reader okay? The two share almost identical implementations. On Mar 18, 2014 11:31 PM, ...

11 years, 9 months ago (2014-03-19 06:38:13 UTC) #2

Is bytes.Reader okay? The two share almost identical implementations.
 On Mar 18, 2014 11:31 PM, <ruiu@google.com> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> strings: fix Reader.UnreadRune
>
> UnreadRune should return an error if previous operation is not
> ReadRune.
>
> Fixes issue 7579.
>
> Please review this at https://codereview.appspot.com/77580046/
>
> Affected files (+28, -2 lines):
>   M src/pkg/strings/reader.go
>   M src/pkg/strings/strings_test.go
>
>
> Index: src/pkg/strings/reader.go
> ===================================================================
> --- a/src/pkg/strings/reader.go
> +++ b/src/pkg/strings/reader.go
> @@ -29,6 +29,7 @@
>  }
>
>  func (r *Reader) Read(b []byte) (n int, err error) {
> +       r.prevRune = -1
>         if len(b) == 0 {
>                 return 0, nil
>         }
> @@ -37,11 +38,11 @@
>         }
>         n = copy(b, r.s[r.i:])
>         r.i += n
> -       r.prevRune = -1
>         return
>  }
>
>  func (r *Reader) ReadAt(b []byte, off int64) (n int, err error) {
> +       r.prevRune = -1
>         if off < 0 {
>                 return 0, errors.New("strings: invalid offset")
>         }
> @@ -56,12 +57,12 @@
>  }
>
>  func (r *Reader) ReadByte() (b byte, err error) {
> +       r.prevRune = -1
>         if r.i >= len(r.s) {
>                 return 0, io.EOF
>         }
>         b = r.s[r.i]
>         r.i++
> -       r.prevRune = -1
>         return
>  }
>
> @@ -76,6 +77,7 @@
>
>  func (r *Reader) ReadRune() (ch rune, size int, err error) {
>         if r.i >= len(r.s) {
> +               r.prevRune = -1
>                 return 0, 0, io.EOF
>         }
>         r.prevRune = r.i
> @@ -99,6 +101,7 @@
>
>  // Seek implements the io.Seeker interface.
>  func (r *Reader) Seek(offset int64, whence int) (int64, error) {
> +       r.prevRune = -1
>         var abs int64
>         switch whence {
>         case 0:
> Index: src/pkg/strings/strings_test.go
> ===================================================================
> --- a/src/pkg/strings/strings_test.go
> +++ b/src/pkg/strings/strings_test.go
> @@ -858,6 +858,29 @@
>         }
>  }
>
> +var ReadRuneErrorTests = []struct {
> +       name string
> +       f    func(*Reader)
> +}{
> +       {"Read", func(r *Reader) { r.Read([]byte{}) }},
> +       {"ReadAt", func(r *Reader) { r.ReadAt([]byte{}, 0) }},
> +       {"ReadByte", func(r *Reader) { r.ReadByte() }},
> +       {"Seek", func(r *Reader) { r.Seek(0, 1) }},
> +       {"WriteTo", func(r *Reader) { r.WriteTo(&bytes.Buffer{}) }},
> +}
> +
> +func TestReadRuneError(t *testing.T) {
> +       for _, tt := range ReadRuneErrorTests {
> +               reader := NewReader("0123456789")
> +               reader.ReadRune()
> +               tt.f(reader)
> +               err := reader.UnreadRune()
> +               if err == nil {
> +                       t.Errorf("Unreading after %s: expected error",
> tt.name)
> +               }
> +       }
> +}
> +
>  var ReplaceTests = []struct {
>         in       string
>         old, new string
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

Sign in to reply to this message.

ruiu

Ah, no. I'll apply this change to bytes.Reader as well. Thank you for pointing it ...

11 years, 9 months ago (2014-03-19 06:40:02 UTC) #3

Ah, no. I'll apply this change to bytes.Reader as well. Thank you for
pointing it out.


On Tue, Mar 18, 2014 at 11:38 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> Is bytes.Reader okay? The two share almost identical implementations.
>  On Mar 18, 2014 11:31 PM, <ruiu@google.com> wrote:
>
>>  Reviewers: golang-codereviews,
>>
>> Message:
>> Hello golang-codereviews@googlegroups.com,
>>
>> I'd like you to review this change to
>> https://code.google.com/p/go
>>
>>
>> Description:
>> strings: fix Reader.UnreadRune
>>
>> UnreadRune should return an error if previous operation is not
>> ReadRune.
>>
>> Fixes issue 7579.
>>
>> Please review this at https://codereview.appspot.com/77580046/
>>
>> Affected files (+28, -2 lines):
>>   M src/pkg/strings/reader.go
>>   M src/pkg/strings/strings_test.go
>>
>>
>> Index: src/pkg/strings/reader.go
>> ===================================================================
>> --- a/src/pkg/strings/reader.go
>> +++ b/src/pkg/strings/reader.go
>> @@ -29,6 +29,7 @@
>>  }
>>
>>  func (r *Reader) Read(b []byte) (n int, err error) {
>> +       r.prevRune = -1
>>         if len(b) == 0 {
>>                 return 0, nil
>>         }
>> @@ -37,11 +38,11 @@
>>         }
>>         n = copy(b, r.s[r.i:])
>>         r.i += n
>> -       r.prevRune = -1
>>         return
>>  }
>>
>>  func (r *Reader) ReadAt(b []byte, off int64) (n int, err error) {
>> +       r.prevRune = -1
>>         if off < 0 {
>>                 return 0, errors.New("strings: invalid offset")
>>         }
>> @@ -56,12 +57,12 @@
>>  }
>>
>>  func (r *Reader) ReadByte() (b byte, err error) {
>> +       r.prevRune = -1
>>         if r.i >= len(r.s) {
>>                 return 0, io.EOF
>>         }
>>         b = r.s[r.i]
>>         r.i++
>> -       r.prevRune = -1
>>         return
>>  }
>>
>> @@ -76,6 +77,7 @@
>>
>>  func (r *Reader) ReadRune() (ch rune, size int, err error) {
>>         if r.i >= len(r.s) {
>> +               r.prevRune = -1
>>                 return 0, 0, io.EOF
>>         }
>>         r.prevRune = r.i
>> @@ -99,6 +101,7 @@
>>
>>  // Seek implements the io.Seeker interface.
>>  func (r *Reader) Seek(offset int64, whence int) (int64, error) {
>> +       r.prevRune = -1
>>         var abs int64
>>         switch whence {
>>         case 0:
>> Index: src/pkg/strings/strings_test.go
>> ===================================================================
>> --- a/src/pkg/strings/strings_test.go
>> +++ b/src/pkg/strings/strings_test.go
>> @@ -858,6 +858,29 @@
>>         }
>>  }
>>
>> +var ReadRuneErrorTests = []struct {
>> +       name string
>> +       f    func(*Reader)
>> +}{
>> +       {"Read", func(r *Reader) { r.Read([]byte{}) }},
>> +       {"ReadAt", func(r *Reader) { r.ReadAt([]byte{}, 0) }},
>> +       {"ReadByte", func(r *Reader) { r.ReadByte() }},
>> +       {"Seek", func(r *Reader) { r.Seek(0, 1) }},
>> +       {"WriteTo", func(r *Reader) { r.WriteTo(&bytes.Buffer{}) }},
>> +}
>> +
>> +func TestReadRuneError(t *testing.T) {
>> +       for _, tt := range ReadRuneErrorTests {
>> +               reader := NewReader("0123456789")
>> +               reader.ReadRune()
>> +               tt.f(reader)
>> +               err := reader.UnreadRune()
>> +               if err == nil {
>> +                       t.Errorf("Unreading after %s: expected error",
>> tt.name)
>> +               }
>> +       }
>> +}
>> +
>>  var ReplaceTests = []struct {
>>         in       string
>>         old, new string
>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "golang-codereviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to golang-codereviews+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/d/optout.
>>
>

Sign in to reply to this message.

bradfitz

Thanks. I will review tomorrow. On Mar 18, 2014 11:40 PM, "Rui Ueyama" <ruiu@google.com> wrote: ...

11 years, 9 months ago (2014-03-19 06:41:49 UTC) #4

Thanks. I will review tomorrow.
On Mar 18, 2014 11:40 PM, "Rui Ueyama" <ruiu@google.com> wrote:

> Ah, no. I'll apply this change to bytes.Reader as well. Thank you for
> pointing it out.
>
>
> On Tue, Mar 18, 2014 at 11:38 PM, Brad Fitzpatrick
<bradfitz@golang.org>wrote:
>
>> Is bytes.Reader okay? The two share almost identical implementations.
>>  On Mar 18, 2014 11:31 PM, <ruiu@google.com> wrote:
>>
>>>  Reviewers: golang-codereviews,
>>>
>>> Message:
>>> Hello golang-codereviews@googlegroups.com,
>>>
>>> I'd like you to review this change to
>>> https://code.google.com/p/go
>>>
>>>
>>> Description:
>>> strings: fix Reader.UnreadRune
>>>
>>> UnreadRune should return an error if previous operation is not
>>> ReadRune.
>>>
>>> Fixes issue 7579.
>>>
>>> Please review this at https://codereview.appspot.com/77580046/
>>>
>>> Affected files (+28, -2 lines):
>>>   M src/pkg/strings/reader.go
>>>   M src/pkg/strings/strings_test.go
>>>
>>>
>>> Index: src/pkg/strings/reader.go
>>> ===================================================================
>>> --- a/src/pkg/strings/reader.go
>>> +++ b/src/pkg/strings/reader.go
>>> @@ -29,6 +29,7 @@
>>>  }
>>>
>>>  func (r *Reader) Read(b []byte) (n int, err error) {
>>> +       r.prevRune = -1
>>>         if len(b) == 0 {
>>>                 return 0, nil
>>>         }
>>> @@ -37,11 +38,11 @@
>>>         }
>>>         n = copy(b, r.s[r.i:])
>>>         r.i += n
>>> -       r.prevRune = -1
>>>         return
>>>  }
>>>
>>>  func (r *Reader) ReadAt(b []byte, off int64) (n int, err error) {
>>> +       r.prevRune = -1
>>>         if off < 0 {
>>>                 return 0, errors.New("strings: invalid offset")
>>>         }
>>> @@ -56,12 +57,12 @@
>>>  }
>>>
>>>  func (r *Reader) ReadByte() (b byte, err error) {
>>> +       r.prevRune = -1
>>>         if r.i >= len(r.s) {
>>>                 return 0, io.EOF
>>>         }
>>>         b = r.s[r.i]
>>>         r.i++
>>> -       r.prevRune = -1
>>>         return
>>>  }
>>>
>>> @@ -76,6 +77,7 @@
>>>
>>>  func (r *Reader) ReadRune() (ch rune, size int, err error) {
>>>         if r.i >= len(r.s) {
>>> +               r.prevRune = -1
>>>                 return 0, 0, io.EOF
>>>         }
>>>         r.prevRune = r.i
>>> @@ -99,6 +101,7 @@
>>>
>>>  // Seek implements the io.Seeker interface.
>>>  func (r *Reader) Seek(offset int64, whence int) (int64, error) {
>>> +       r.prevRune = -1
>>>         var abs int64
>>>         switch whence {
>>>         case 0:
>>> Index: src/pkg/strings/strings_test.go
>>> ===================================================================
>>> --- a/src/pkg/strings/strings_test.go
>>> +++ b/src/pkg/strings/strings_test.go
>>> @@ -858,6 +858,29 @@
>>>         }
>>>  }
>>>
>>> +var ReadRuneErrorTests = []struct {
>>> +       name string
>>> +       f    func(*Reader)
>>> +}{
>>> +       {"Read", func(r *Reader) { r.Read([]byte{}) }},
>>> +       {"ReadAt", func(r *Reader) { r.ReadAt([]byte{}, 0) }},
>>> +       {"ReadByte", func(r *Reader) { r.ReadByte() }},
>>> +       {"Seek", func(r *Reader) { r.Seek(0, 1) }},
>>> +       {"WriteTo", func(r *Reader) { r.WriteTo(&bytes.Buffer{}) }},
>>> +}
>>> +
>>> +func TestReadRuneError(t *testing.T) {
>>> +       for _, tt := range ReadRuneErrorTests {
>>> +               reader := NewReader("0123456789")
>>> +               reader.ReadRune()
>>> +               tt.f(reader)
>>> +               err := reader.UnreadRune()
>>> +               if err == nil {
>>> +                       t.Errorf("Unreading after %s: expected error",
>>> tt.name)
>>> +               }
>>> +       }
>>> +}
>>> +
>>>  var ReplaceTests = []struct {
>>>         in       string
>>>         old, new string
>>>
>>>
>>> --
>>> You received this message because you are subscribed to the Google
>>> Groups "golang-codereviews" group.
>>> To unsubscribe from this group and stop receiving emails from it, send
>>> an email to golang-codereviews+unsubscribe@googlegroups.com.
>>> For more options, visit https://groups.google.com/d/optout.
>>>
>>
>

Sign in to reply to this message.

ruiu

Hello golang-codereviews@googlegroups.com, bradfitz@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.

11 years, 9 months ago (2014-03-19 06:51:56 UTC) #5

bradfitz

*** Submitted as https://code.google.com/p/go/source/detail?r=358e2b416518 *** strings, bytes: fix Reader.UnreadRune UnreadRune should return an error if ...

11 years, 9 months ago (2014-03-19 16:01:01 UTC) #7

gobot

This CL appears to have broken the linux-amd64-race builder. See http://build.golang.org/log/2d924df645a4978ae681f709fdb30630554d42aa

11 years, 9 months ago (2014-03-19 16:05:05 UTC) #8

josharian

That breakage looks real, and related: ================== WARNING: DATA RACE Write by goroutine 26: bytes.(*Reader).ReadAt() ...

11 years, 9 months ago (2014-03-19 18:10:40 UTC) #9

That breakage looks real, and related:

==================
WARNING: DATA RACE
Write by goroutine 26:
  bytes.(*Reader).ReadAt()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/reader.go:46
+0x8a
  archive/zip.(*File).findBodyOffset()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:196
+0x129
  archive/zip.(*File).Open()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:133
+0xb4
  archive/zip.readTestFile()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:359
+0x511
  archive/zip.func·003()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:329
+0x182

Previous write by goroutine 25:
  bytes.(*Reader).ReadAt()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/reader.go:46
+0x8a
  io.(*SectionReader).Read()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/io/io.go:425
+0x181
  bufio.(*Reader).fill()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bufio/bufio.go:91
+0x27e
  bufio.(*Reader).ReadByte()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bufio/bufio.go:183
+0x159
  compress/flate.(*decompressor).moreBits()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/compress/flate/inflate.go:620
+0x78
  compress/flate.(*decompressor).nextBlock()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/compress/flate/inflate.go:234
+0x156
  compress/flate.(*decompressor).Read()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/compress/flate/inflate.go:275
+0x2da
  archive/zip.(*checksumReader).Read()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:165
+0x133
  bytes.(*Buffer).ReadFrom()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/buffer.go:169
+0x439
  io.Copy()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/io/io.go:348
+0x168
  archive/zip.readTestFile()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:365
+0x70c
  archive/zip.func·003()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:329
+0x182

Goroutine 26 (running) created at:
  archive/zip.readTestZip()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:331
+0x970
  archive/zip.TestReader()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:277
+0x10d
  testing.tRunner()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/testing/testing.go:422
+0x121

Goroutine 25 (running) created at:
  archive/zip.readTestZip()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:331
+0x970
  archive/zip.TestReader()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:277
+0x10d
  testing.tRunner()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/testing/testing.go:422
+0x121
==================
==================
WARNING: DATA RACE
Write by goroutine 16:
  bytes.(*Reader).ReadAt()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/reader.go:46
+0x8a
  archive/zip.(*File).findBodyOffset()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:196
+0x129
  archive/zip.(*File).Open()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:133
+0xb4
  archive/zip.readTestFile()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:359
+0x511
  archive/zip.func·003()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:329
+0x182

Previous write by goroutine 15:
  bytes.(*Reader).ReadAt()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/reader.go:46
+0x8a
  io.(*SectionReader).Read()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/io/io.go:425
+0x181
  io/ioutil.(*nopCloser).Read()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/io/ioutil/ioutil.go:1
+0x9b
  archive/zip.(*checksumReader).Read()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:165
+0x133
  bytes.(*Buffer).ReadFrom()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/buffer.go:169
+0x439
  io.Copy()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/io/io.go:348
+0x168
  archive/zip.readTestFile()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:365
+0x70c
  archive/zip.func·003()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:329
+0x182

Goroutine 16 (running) created at:
  archive/zip.readTestZip()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:331
+0x970
  archive/zip.TestReader()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:277
+0x10d
  testing.tRunner()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/testing/testing.go:422
+0x121

Goroutine 15 (finished) created at:
  archive/zip.readTestZip()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:331
+0x970
  archive/zip.TestReader()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:277
+0x10d
  testing.tRunner()
     
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/testing/testing.go:422
+0x121
==================
PASS
Found 2 data race(s)

On Wed, Mar 19, 2014 at 12:05 PM,  <gobot@golang.org> wrote:
> This CL appears to have broken the linux-amd64-race builder.
> See http://build.golang.org/log/2d924df645a4978ae681f709fdb30630554d42aa
>
>
> https://codereview.appspot.com/77580046/
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

Sign in to reply to this message.

ruiu

Does this buildbot have a special flag to enable data race detection? I can't reproduce ...

11 years, 9 months ago (2014-03-19 18:21:09 UTC) #10

Does this buildbot have a special flag to enable data race detection? I
can't reproduce it locally.


On Wed, Mar 19, 2014 at 11:07 AM, Josh Bleecher Snyder
<josharian@gmail.com>wrote:

> That breakage looks real, and related:
>
> ==================
> WARNING: DATA RACE
> Write by goroutine 26:
>   bytes.(*Reader).ReadAt()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/reader.go:46
> +0x8a
>   archive/zip.(*File).findBodyOffset()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:196
> +0x129
>   archive/zip.(*File).Open()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:133
> +0xb4
>   archive/zip.readTestFile()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:359
> +0x511
>   archive/zip.func·003()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:329
> +0x182
>
> Previous write by goroutine 25:
>   bytes.(*Reader).ReadAt()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/reader.go:46
> +0x8a
>   io.(*SectionReader).Read()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/io/io.go:425
> +0x181
>   bufio.(*Reader).fill()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bufio/bufio.go:91
> +0x27e
>   bufio.(*Reader).ReadByte()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bufio/bufio.go:183
> +0x159
>   compress/flate.(*decompressor).moreBits()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/compress/flate/inflate.go:620
> +0x78
>   compress/flate.(*decompressor).nextBlock()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/compress/flate/inflate.go:234
> +0x156
>   compress/flate.(*decompressor).Read()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/compress/flate/inflate.go:275
> +0x2da
>   archive/zip.(*checksumReader).Read()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:165
> +0x133
>   bytes.(*Buffer).ReadFrom()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/buffer.go:169
> +0x439
>   io.Copy()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/io/io.go:348
> +0x168
>   archive/zip.readTestFile()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:365
> +0x70c
>   archive/zip.func·003()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:329
> +0x182
>
> Goroutine 26 (running) created at:
>   archive/zip.readTestZip()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:331
> +0x970
>   archive/zip.TestReader()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:277
> +0x10d
>   testing.tRunner()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/testing/testing.go:422
> +0x121
>
> Goroutine 25 (running) created at:
>   archive/zip.readTestZip()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:331
> +0x970
>   archive/zip.TestReader()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:277
> +0x10d
>   testing.tRunner()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/testing/testing.go:422
> +0x121
> ==================
> ==================
> WARNING: DATA RACE
> Write by goroutine 16:
>   bytes.(*Reader).ReadAt()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/reader.go:46
> +0x8a
>   archive/zip.(*File).findBodyOffset()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:196
> +0x129
>   archive/zip.(*File).Open()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:133
> +0xb4
>   archive/zip.readTestFile()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:359
> +0x511
>   archive/zip.func·003()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:329
> +0x182
>
> Previous write by goroutine 15:
>   bytes.(*Reader).ReadAt()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/reader.go:46
> +0x8a
>   io.(*SectionReader).Read()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/io/io.go:425
> +0x181
>   io/ioutil.(*nopCloser).Read()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/io/ioutil/ioutil.go:1
> +0x9b
>   archive/zip.(*checksumReader).Read()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:165
> +0x133
>   bytes.(*Buffer).ReadFrom()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/buffer.go:169
> +0x439
>   io.Copy()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/io/io.go:348
> +0x168
>   archive/zip.readTestFile()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:365
> +0x70c
>   archive/zip.func·003()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:329
> +0x182
>
> Goroutine 16 (running) created at:
>   archive/zip.readTestZip()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:331
> +0x970
>   archive/zip.TestReader()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:277
> +0x10d
>   testing.tRunner()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/testing/testing.go:422
> +0x121
>
> Goroutine 15 (finished) created at:
>   archive/zip.readTestZip()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:331
> +0x970
>   archive/zip.TestReader()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:277
> +0x10d
>   testing.tRunner()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/testing/testing.go:422
> +0x121
> ==================
> PASS
> Found 2 data race(s)
>
> On Wed, Mar 19, 2014 at 12:05 PM,  <gobot@golang.org> wrote:
> > This CL appears to have broken the linux-amd64-race builder.
> > See http://build.golang.org/log/2d924df645a4978ae681f709fdb30630554d42aa
> >
> >
> > https://codereview.appspot.com/77580046/
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "golang-codereviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> > email to golang-codereviews+unsubscribe@googlegroups.com.
> > For more options, visit https://groups.google.com/d/optout.
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

Sign in to reply to this message.

josharian

Try: go test -race archive On Wed, Mar 19, 2014 at 2:20 PM, Rui Ueyama ...

11 years, 9 months ago (2014-03-19 18:28:12 UTC) #11

Try:

go test -race archive


On Wed, Mar 19, 2014 at 2:20 PM, Rui Ueyama <ruiu@google.com> wrote:
> Does this buildbot have a special flag to enable data race detection? I
> can't reproduce it locally.
>
>
> On Wed, Mar 19, 2014 at 11:07 AM, Josh Bleecher Snyder <josharian@gmail.com>
> wrote:
>>
>> That breakage looks real, and related:
>>
>> ==================
>> WARNING: DATA RACE
>> Write by goroutine 26:
>>   bytes.(*Reader).ReadAt()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/reader.go:46
>> +0x8a
>>   archive/zip.(*File).findBodyOffset()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:196
>> +0x129
>>   archive/zip.(*File).Open()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:133
>> +0xb4
>>   archive/zip.readTestFile()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:359
>> +0x511
>>   archive/zip.func·003()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:329
>> +0x182
>>
>> Previous write by goroutine 25:
>>   bytes.(*Reader).ReadAt()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/reader.go:46
>> +0x8a
>>   io.(*SectionReader).Read()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/io/io.go:425
>> +0x181
>>   bufio.(*Reader).fill()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bufio/bufio.go:91
>> +0x27e
>>   bufio.(*Reader).ReadByte()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bufio/bufio.go:183
>> +0x159
>>   compress/flate.(*decompressor).moreBits()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/compress/flate/inflate.go:620
>> +0x78
>>   compress/flate.(*decompressor).nextBlock()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/compress/flate/inflate.go:234
>> +0x156
>>   compress/flate.(*decompressor).Read()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/compress/flate/inflate.go:275
>> +0x2da
>>   archive/zip.(*checksumReader).Read()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:165
>> +0x133
>>   bytes.(*Buffer).ReadFrom()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/buffer.go:169
>> +0x439
>>   io.Copy()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/io/io.go:348
>> +0x168
>>   archive/zip.readTestFile()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:365
>> +0x70c
>>   archive/zip.func·003()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:329
>> +0x182
>>
>> Goroutine 26 (running) created at:
>>   archive/zip.readTestZip()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:331
>> +0x970
>>   archive/zip.TestReader()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:277
>> +0x10d
>>   testing.tRunner()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/testing/testing.go:422
>> +0x121
>>
>> Goroutine 25 (running) created at:
>>   archive/zip.readTestZip()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:331
>> +0x970
>>   archive/zip.TestReader()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:277
>> +0x10d
>>   testing.tRunner()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/testing/testing.go:422
>> +0x121
>> ==================
>> ==================
>> WARNING: DATA RACE
>> Write by goroutine 16:
>>   bytes.(*Reader).ReadAt()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/reader.go:46
>> +0x8a
>>   archive/zip.(*File).findBodyOffset()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:196
>> +0x129
>>   archive/zip.(*File).Open()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:133
>> +0xb4
>>   archive/zip.readTestFile()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:359
>> +0x511
>>   archive/zip.func·003()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:329
>> +0x182
>>
>> Previous write by goroutine 15:
>>   bytes.(*Reader).ReadAt()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/reader.go:46
>> +0x8a
>>   io.(*SectionReader).Read()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/io/io.go:425
>> +0x181
>>   io/ioutil.(*nopCloser).Read()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/io/ioutil/ioutil.go:1
>> +0x9b
>>   archive/zip.(*checksumReader).Read()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:165
>> +0x133
>>   bytes.(*Buffer).ReadFrom()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/buffer.go:169
>> +0x439
>>   io.Copy()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/io/io.go:348
>> +0x168
>>   archive/zip.readTestFile()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:365
>> +0x70c
>>   archive/zip.func·003()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:329
>> +0x182
>>
>> Goroutine 16 (running) created at:
>>   archive/zip.readTestZip()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:331
>> +0x970
>>   archive/zip.TestReader()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:277
>> +0x10d
>>   testing.tRunner()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/testing/testing.go:422
>> +0x121
>>
>> Goroutine 15 (finished) created at:
>>   archive/zip.readTestZip()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:331
>> +0x970
>>   archive/zip.TestReader()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:277
>> +0x10d
>>   testing.tRunner()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/testing/testing.go:422
>> +0x121
>> ==================
>> PASS
>> Found 2 data race(s)
>>
>> On Wed, Mar 19, 2014 at 12:05 PM,  <gobot@golang.org> wrote:
>> > This CL appears to have broken the linux-amd64-race builder.
>> > See http://build.golang.org/log/2d924df645a4978ae681f709fdb30630554d42aa
>> >
>> >
>> > https://codereview.appspot.com/77580046/
>> >
>> > --
>> > You received this message because you are subscribed to the Google
>> > Groups
>> > "golang-codereviews" group.
>> > To unsubscribe from this group and stop receiving emails from it, send
>> > an
>> > email to golang-codereviews+unsubscribe@googlegroups.com.
>> > For more options, visit https://groups.google.com/d/optout.
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "golang-codereviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to golang-codereviews+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/d/optout.
>
>

Sign in to reply to this message.

ruiu

It looks like the reason of the failure is that the test does not assume ...

11 years, 9 months ago (2014-03-19 18:31:49 UTC) #12

It looks like the reason of the failure is that the test does not assume
that ReadAt will mutate the receiver. After my patch, ReadAt sets -1 to
unread rune variable, which I believe caused this breakage.

Should we allow UnreadRune after ReadAt? If we allow it ReadAt does not
have to alter the state of the receiver.


On Wed, Mar 19, 2014 at 11:20 AM, Rui Ueyama <ruiu@google.com> wrote:

> Does this buildbot have a special flag to enable data race detection? I
> can't reproduce it locally.
>
>
> On Wed, Mar 19, 2014 at 11:07 AM, Josh Bleecher Snyder <
> josharian@gmail.com> wrote:
>
>> That breakage looks real, and related:
>>
>> ==================
>> WARNING: DATA RACE
>> Write by goroutine 26:
>>   bytes.(*Reader).ReadAt()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/reader.go:46
>> +0x8a
>>   archive/zip.(*File).findBodyOffset()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:196
>> +0x129
>>   archive/zip.(*File).Open()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:133
>> +0xb4
>>   archive/zip.readTestFile()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:359
>> +0x511
>>   archive/zip.func·003()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:329
>> +0x182
>>
>> Previous write by goroutine 25:
>>   bytes.(*Reader).ReadAt()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/reader.go:46
>> +0x8a
>>   io.(*SectionReader).Read()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/io/io.go:425
>> +0x181
>>   bufio.(*Reader).fill()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bufio/bufio.go:91
>> +0x27e
>>   bufio.(*Reader).ReadByte()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bufio/bufio.go:183
>> +0x159
>>   compress/flate.(*decompressor).moreBits()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/compress/flate/inflate.go:620
>> +0x78
>>   compress/flate.(*decompressor).nextBlock()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/compress/flate/inflate.go:234
>> +0x156
>>   compress/flate.(*decompressor).Read()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/compress/flate/inflate.go:275
>> +0x2da
>>   archive/zip.(*checksumReader).Read()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:165
>> +0x133
>>   bytes.(*Buffer).ReadFrom()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/buffer.go:169
>> +0x439
>>   io.Copy()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/io/io.go:348
>> +0x168
>>   archive/zip.readTestFile()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:365
>> +0x70c
>>   archive/zip.func·003()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:329
>> +0x182
>>
>> Goroutine 26 (running) created at:
>>   archive/zip.readTestZip()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:331
>> +0x970
>>   archive/zip.TestReader()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:277
>> +0x10d
>>   testing.tRunner()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/testing/testing.go:422
>> +0x121
>>
>> Goroutine 25 (running) created at:
>>   archive/zip.readTestZip()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:331
>> +0x970
>>   archive/zip.TestReader()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:277
>> +0x10d
>>   testing.tRunner()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/testing/testing.go:422
>> +0x121
>> ==================
>> ==================
>> WARNING: DATA RACE
>> Write by goroutine 16:
>>   bytes.(*Reader).ReadAt()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/reader.go:46
>> +0x8a
>>   archive/zip.(*File).findBodyOffset()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:196
>> +0x129
>>   archive/zip.(*File).Open()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:133
>> +0xb4
>>   archive/zip.readTestFile()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:359
>> +0x511
>>   archive/zip.func·003()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:329
>> +0x182
>>
>> Previous write by goroutine 15:
>>   bytes.(*Reader).ReadAt()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/reader.go:46
>> +0x8a
>>   io.(*SectionReader).Read()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/io/io.go:425
>> +0x181
>>   io/ioutil.(*nopCloser).Read()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/io/ioutil/ioutil.go:1
>> +0x9b
>>   archive/zip.(*checksumReader).Read()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:165
>> +0x133
>>   bytes.(*Buffer).ReadFrom()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/buffer.go:169
>> +0x439
>>   io.Copy()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/io/io.go:348
>> +0x168
>>   archive/zip.readTestFile()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:365
>> +0x70c
>>   archive/zip.func·003()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:329
>> +0x182
>>
>> Goroutine 16 (running) created at:
>>   archive/zip.readTestZip()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:331
>> +0x970
>>   archive/zip.TestReader()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:277
>> +0x10d
>>   testing.tRunner()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/testing/testing.go:422
>> +0x121
>>
>> Goroutine 15 (finished) created at:
>>   archive/zip.readTestZip()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:331
>> +0x970
>>   archive/zip.TestReader()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:277
>> +0x10d
>>   testing.tRunner()
>>
>>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/testing/testing.go:422
>> +0x121
>> ==================
>> PASS
>> Found 2 data race(s)
>>
>> On Wed, Mar 19, 2014 at 12:05 PM,  <gobot@golang.org> wrote:
>> > This CL appears to have broken the linux-amd64-race builder.
>> > See
>> http://build.golang.org/log/2d924df645a4978ae681f709fdb30630554d42aa
>> >
>> >
>> > https://codereview.appspot.com/77580046/
>> >
>> > --
>> > You received this message because you are subscribed to the Google
>> Groups
>> > "golang-codereviews" group.
>> > To unsubscribe from this group and stop receiving emails from it, send
>> an
>> > email to golang-codereviews+unsubscribe@googlegroups.com.
>> > For more options, visit https://groups.google.com/d/optout.
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "golang-codereviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to golang-codereviews+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/d/optout.
>>
>
>

Sign in to reply to this message.

bradfitz

Yes, this is real. Any io.ReaderAt is allowed to be called concurrently from many goroutines. ...

11 years, 9 months ago (2014-03-19 18:31:57 UTC) #13

Yes, this is real.

Any io.ReaderAt is allowed to be called concurrently from many goroutines.
We can't mutate {strings,bytes}.Reader's fields in ReadAt calls without
locks/atomics.  But I don't think it's worth adding in locks & atomics
everywhere, just for UnreadRune's error value.  Better to simply document
that.

I propose we just remove the lastRune = -1 for the ReadAt case.



On Wed, Mar 19, 2014 at 11:07 AM, Josh Bleecher Snyder
<josharian@gmail.com>wrote:

> That breakage looks real, and related:
>
> ==================
> WARNING: DATA RACE
> Write by goroutine 26:
>   bytes.(*Reader).ReadAt()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/reader.go:46
> +0x8a
>   archive/zip.(*File).findBodyOffset()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:196
> +0x129
>   archive/zip.(*File).Open()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:133
> +0xb4
>   archive/zip.readTestFile()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:359
> +0x511
>   archive/zip.func·003()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:329
> +0x182
>
> Previous write by goroutine 25:
>   bytes.(*Reader).ReadAt()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/reader.go:46
> +0x8a
>   io.(*SectionReader).Read()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/io/io.go:425
> +0x181
>   bufio.(*Reader).fill()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bufio/bufio.go:91
> +0x27e
>   bufio.(*Reader).ReadByte()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bufio/bufio.go:183
> +0x159
>   compress/flate.(*decompressor).moreBits()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/compress/flate/inflate.go:620
> +0x78
>   compress/flate.(*decompressor).nextBlock()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/compress/flate/inflate.go:234
> +0x156
>   compress/flate.(*decompressor).Read()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/compress/flate/inflate.go:275
> +0x2da
>   archive/zip.(*checksumReader).Read()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:165
> +0x133
>   bytes.(*Buffer).ReadFrom()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/buffer.go:169
> +0x439
>   io.Copy()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/io/io.go:348
> +0x168
>   archive/zip.readTestFile()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:365
> +0x70c
>   archive/zip.func·003()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:329
> +0x182
>
> Goroutine 26 (running) created at:
>   archive/zip.readTestZip()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:331
> +0x970
>   archive/zip.TestReader()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:277
> +0x10d
>   testing.tRunner()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/testing/testing.go:422
> +0x121
>
> Goroutine 25 (running) created at:
>   archive/zip.readTestZip()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:331
> +0x970
>   archive/zip.TestReader()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:277
> +0x10d
>   testing.tRunner()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/testing/testing.go:422
> +0x121
> ==================
> ==================
> WARNING: DATA RACE
> Write by goroutine 16:
>   bytes.(*Reader).ReadAt()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/reader.go:46
> +0x8a
>   archive/zip.(*File).findBodyOffset()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:196
> +0x129
>   archive/zip.(*File).Open()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:133
> +0xb4
>   archive/zip.readTestFile()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:359
> +0x511
>   archive/zip.func·003()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:329
> +0x182
>
> Previous write by goroutine 15:
>   bytes.(*Reader).ReadAt()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/reader.go:46
> +0x8a
>   io.(*SectionReader).Read()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/io/io.go:425
> +0x181
>   io/ioutil.(*nopCloser).Read()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/io/ioutil/ioutil.go:1
> +0x9b
>   archive/zip.(*checksumReader).Read()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader.go:165
> +0x133
>   bytes.(*Buffer).ReadFrom()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/bytes/buffer.go:169
> +0x439
>   io.Copy()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/io/io.go:348
> +0x168
>   archive/zip.readTestFile()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:365
> +0x70c
>   archive/zip.func·003()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:329
> +0x182
>
> Goroutine 16 (running) created at:
>   archive/zip.readTestZip()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:331
> +0x970
>   archive/zip.TestReader()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:277
> +0x10d
>   testing.tRunner()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/testing/testing.go:422
> +0x121
>
> Goroutine 15 (finished) created at:
>   archive/zip.readTestZip()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:331
> +0x970
>   archive/zip.TestReader()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/archive/zip/reader_test.go:277
> +0x10d
>   testing.tRunner()
>
>
/home/dvyukov/racer/work/linux-amd64-race-358e2b416518/go/src/pkg/testing/testing.go:422
> +0x121
> ==================
> PASS
> Found 2 data race(s)
>
> On Wed, Mar 19, 2014 at 12:05 PM,  <gobot@golang.org> wrote:
> > This CL appears to have broken the linux-amd64-race builder.
> > See http://build.golang.org/log/2d924df645a4978ae681f709fdb30630554d42aa
> >
> >
> > https://codereview.appspot.com/77580046/
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "golang-codereviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> > email to golang-codereviews+unsubscribe@googlegroups.com.
> > For more options, visit https://groups.google.com/d/optout.
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

Sign in to reply to this message.

bradfitz

On Wed, Mar 19, 2014 at 11:31 AM, Rui Ueyama <ruiu@google.com> wrote: > It looks ...

11 years, 9 months ago (2014-03-19 18:32:54 UTC) #14

ruiu

11 years, 9 months ago (2014-03-19 18:34:10 UTC) #15

Thanks, Brad.

I'll send you a patch to fix it now. Hold on.

Sorry for the breakage.


On Wed, Mar 19, 2014 at 11:32 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

>
>
>
> On Wed, Mar 19, 2014 at 11:31 AM, Rui Ueyama <ruiu@google.com> wrote:
>
>> It looks like the reason of the failure is that the test does not assume
>> that ReadAt will mutate the receiver. After my patch, ReadAt sets -1 to
>> unread rune variable, which I believe caused this breakage.
>>
>> Should we allow UnreadRune after ReadAt? If we allow it ReadAt does not
>> have to alter the state of the receiver.
>>
>
> Yes, ReadAt doesn't mutate any state (notably the offset or consume any
> data), so that's fine to call ReadAt then UnreadRune.
>
>
>

Sign in to reply to this message.

Issue 77580046: code review 77580046: strings: fix Reader.UnreadRune (Closed)

Description

Patch Set 1 #

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

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

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

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

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

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

Messages