code review 77580046: strings: fix Reader.UnreadRune

strings, bytes: fix Reader.UnreadRune

UnreadRune should return an error if previous operation is not
ReadRune.

Fixes issue 7579.

[ruiu, 2014-03-19 06:31:36]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2014-03-19 06:38:13]

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.
>

[ruiu, 2014-03-19 06:40:02]

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.
>>
>

[bradfitz, 2014-03-19 06:41:49]

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.
>>>
>>
>

[ruiu, 2014-03-19 06:51:56]

Hello golang-codereviews@googlegroups.com, bradfitz@golang.org (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[bradfitz, 2014-03-19 15:59:02]

LGTM

[bradfitz, 2014-03-19 16:01:01]

*** Submitted as https://code.google.com/p/go/source/detail?r=358e2b416518 ***

strings, bytes: fix Reader.UnreadRune

UnreadRune should return an error if previous operation is not
ReadRune.

Fixes issue 7579.

LGTM=bradfitz
R=golang-codereviews, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/77580046

Committer: Brad Fitzpatrick <bradfitz@golang.org>

[gobot, 2014-03-19 16:05:05]

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

[josharian, 2014-03-19 18:10:40]

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.

[ruiu, 2014-03-19 18:21:09]

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.
>

[josharian, 2014-03-19 18:28:12]

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.
>
>

[ruiu, 2014-03-19 18:31:49]

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.
>>
>
>

[bradfitz, 2014-03-19 18:31:57]

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.
>

[bradfitz, 2014-03-19 18:32:54]

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.

[ruiu, 2014-03-19 18:34:10]

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.
>
>
>