From 4f180445dd6f685926be0c2f6b180d1f3205a09c Mon Sep 17 00:00:00 2001 From: Mark Kremer Date: Mon, 5 Aug 2024 21:28:32 +0200 Subject: [PATCH 1/4] Make flac.NewSeek() use a buffered reader Which is similar to how flac.New() works. --- flac.go | 6 +- internal/bufseekio/readseeker.go | 152 +++++++++++++++++++++++++++++++ 2 files changed, 156 insertions(+), 2 deletions(-) create mode 100644 internal/bufseekio/readseeker.go diff --git a/flac.go b/flac.go index 20d4531..030090d 100644 --- a/flac.go +++ b/flac.go @@ -35,6 +35,7 @@ import ( "os" "github.com/mewkiz/flac/frame" + "github.com/mewkiz/flac/internal/bufseekio" "github.com/mewkiz/flac/meta" ) @@ -96,7 +97,8 @@ func New(r io.Reader) (stream *Stream, err error) { // will not be buffered, which might result in performance issues. Using an // in-memory buffer like *bytes.Reader should work well. func NewSeek(rs io.ReadSeeker) (stream *Stream, err error) { - stream = &Stream{r: rs, seekTableSize: defaultSeekTableSize} + br := bufseekio.NewReadSeeker(rs) + stream = &Stream{r: br, seekTableSize: defaultSeekTableSize} // Verify FLAC signature and parse the StreamInfo metadata block. block, err := stream.parseStreamInfo() @@ -121,7 +123,7 @@ func NewSeek(rs io.ReadSeeker) (stream *Stream, err error) { } // Record file offset of the first frame header. - stream.dataStart, err = rs.Seek(0, io.SeekCurrent) + stream.dataStart, err = br.Seek(0, io.SeekCurrent) return stream, err } diff --git a/internal/bufseekio/readseeker.go b/internal/bufseekio/readseeker.go new file mode 100644 index 0000000..cf9ce2b --- /dev/null +++ b/internal/bufseekio/readseeker.go @@ -0,0 +1,152 @@ +package bufseekio + +import ( + "errors" + "io" +) + +const ( + defaultBufSize = 4096 +) + +// ReadSeeker implements buffering for an io.ReadSeeker object. +// ReadSeeker is based on bufio.Reader with Seek functionality added +// and unneeded functionality removed. +type ReadSeeker struct { + buf []byte + pos int64 // absolute start position of buf + rd io.ReadSeeker // read-seeker provided by the client + r, w int // buf read and write positions within buf + err error +} + +const minReadBufferSize = 16 + +// NewReadSeekerSize returns a new ReadSeeker whose buffer has at least the specified +// size. If the argument io.ReadSeeker is already a ReadSeeker with large enough +// size, it returns the underlying ReadSeeker. +func NewReadSeekerSize(rd io.ReadSeeker, size int) *ReadSeeker { + // Is it already a Reader? + b, ok := rd.(*ReadSeeker) + if ok && len(b.buf) >= size { + return b + } + if size < minReadBufferSize { + size = minReadBufferSize + } + r := new(ReadSeeker) + r.reset(make([]byte, size), rd) + return r +} + +// NewReadSeeker returns a new ReadSeeker whose buffer has the default size. +func NewReadSeeker(rd io.ReadSeeker) *ReadSeeker { + return NewReadSeekerSize(rd, defaultBufSize) +} + +var errNegativeRead = errors.New("bufseekio: reader returned negative count from Read") + +func (b *ReadSeeker) reset(buf []byte, r io.ReadSeeker) { + *b = ReadSeeker{ + buf: buf, + rd: r, + } +} + +func (b *ReadSeeker) readErr() error { + err := b.err + b.err = nil + return err +} + +// Read reads data into p. +// It returns the number of bytes read into p. +// The bytes are taken from at most one Read on the underlying Reader, +// hence n may be less than len(p). +// To read exactly len(p) bytes, use io.ReadFull(b, p). +// If the underlying Reader can return a non-zero count with io.EOF, +// then this Read method can do so as well; see the [io.Reader] docs. +func (b *ReadSeeker) Read(p []byte) (n int, err error) { + n = len(p) + if n == 0 { + if b.Buffered() > 0 { + return 0, nil + } + return 0, b.readErr() + } + if b.r == b.w { + if b.err != nil { + return 0, b.readErr() + } + if len(p) >= len(b.buf) { + // Large read, empty buffer. + // Read directly into p to avoid copy. + n, b.err = b.rd.Read(p) + if n < 0 { + panic(errNegativeRead) + } + b.pos += int64(n) + return n, b.readErr() + } + // One read. + b.pos += int64(b.r) + b.r = 0 + b.w = 0 + n, b.err = b.rd.Read(b.buf) + if n < 0 { + panic(errNegativeRead) + } + if n == 0 { + return 0, b.readErr() + } + b.w += n + } + + // copy as much as we can + // Note: if the slice panics here, it is probably because + // the underlying reader returned a bad count. See issue 49795. + n = copy(p, b.buf[b.r:b.w]) + b.r += n + return n, nil +} + +// Buffered returns the number of bytes that can be read from the current buffer. +func (b *ReadSeeker) Buffered() int { return b.w - b.r } + +func (b *ReadSeeker) Seek(offset int64, whence int) (int64, error) { + // The stream.Seek() implementation makes heavy use of seeking with offset 0 + // to obtain the current position; let's optimize for it. + if offset == 0 && whence == io.SeekCurrent { + return b.position(), nil + } + // When seeking from the end, the absolute position isn't known by ReadSeeker + // so the current buffer cannot be used. Seeking cannot be avoided. + if whence == io.SeekEnd { + return b.seek(offset, whence) + } + // Calculate the absolute offset. + absOffset := offset + if whence == io.SeekCurrent { + absOffset += b.position() + } + // Check if the offset is within buf. + if absOffset >= b.pos && absOffset < b.pos+int64(b.w) { + b.r = int(absOffset - b.pos) + return absOffset, nil + } + + return b.seek(offset, whence) +} + +func (b *ReadSeeker) seek(offset int64, whence int) (int64, error) { + b.r = 0 + b.w = 0 + var err error + b.pos, err = b.rd.Seek(offset, whence) + return b.pos, err +} + +// position returns the absolute read offset. +func (b *ReadSeeker) position() int64 { + return b.pos + int64(b.r) +} From 65e7a4aed462128a39ad3ba6d0737563246851d9 Mon Sep 17 00:00:00 2001 From: Mark Kremer Date: Tue, 6 Aug 2024 22:06:05 +0200 Subject: [PATCH 2/4] Test ReadSeeker --- internal/bufseekio/readseeker_test.go | 317 ++++++++++++++++++++++++++ 1 file changed, 317 insertions(+) create mode 100644 internal/bufseekio/readseeker_test.go diff --git a/internal/bufseekio/readseeker_test.go b/internal/bufseekio/readseeker_test.go new file mode 100644 index 0000000..951686e --- /dev/null +++ b/internal/bufseekio/readseeker_test.go @@ -0,0 +1,317 @@ +package bufseekio + +import ( + "bytes" + "fmt" + "io" + "reflect" + "testing" +) + +func TestReadSeeker_Seek_SeekCurrent(t *testing.T) { + recorder := &readSeekRecorder{rs: bytes.NewReader(make([]byte, 100))} + + rs := NewReadSeekerSize(recorder, 20) + if len(rs.buf) != 20 { + t.Fatal("the buffer size was changed and the validity of this test has become unknown") + } + + // Seek forwards. + p, err := rs.Seek(10, io.SeekCurrent) + if err != nil { + t.Fatalf("seek error: %v", err) + } + if p != 10 { + t.Fatalf("seek position mismatch: expected %d, got %d", 10, p) + } + recorder.assertSeeks(t, []seekRecord{{offset: 10, whence: io.SeekCurrent}}) + + // Get position without moving. + p, err = rs.Seek(0, io.SeekCurrent) + if err != nil { + t.Fatalf("seek error: %v", err) + } + if p != 10 { + t.Fatalf("seek position mismatch: expected %d, got %d", 10, p) + } + recorder.assertSeeks(t, nil) + + // Move backwards. + p, err = rs.Seek(-5, io.SeekCurrent) + if err != nil { + t.Fatalf("seek error: %v", err) + } + if p != 5 { + t.Fatalf("seek position mismatch: expected %d, got %d", 5, p) + } + recorder.assertSeeks(t, []seekRecord{{offset: -5, whence: io.SeekCurrent}}) +} + +func TestReadSeeker_Seek_SeekEnd(t *testing.T) { + recorder := &readSeekRecorder{rs: bytes.NewReader(make([]byte, 100))} + + rs := NewReadSeekerSize(recorder, 20) + if len(rs.buf) != 20 { + t.Fatal("the buffer size was changed and the validity of this test has become unknown") + } + + // Seek from end. + p, err := rs.Seek(-10, io.SeekEnd) + if err != nil { + t.Fatalf("seek error: %v", err) + } + if p != 90 { + t.Fatalf("seek position mismatch: expected %d, got %d", 90, p) + } + recorder.assertSeeks(t, []seekRecord{{offset: -10, whence: io.SeekEnd}}) + + // Seek from end again. + p, err = rs.Seek(-10, io.SeekEnd) + if err != nil { + t.Fatalf("seek error: %v", err) + } + if p != 90 { + t.Fatalf("seek position mismatch: expected %d, got %d", 90, p) + } + // It will always seek again because it only keeps track of the position from the start. + recorder.assertSeeks(t, []seekRecord{{offset: -10, whence: io.SeekEnd}}) +} + +func TestReadSeeker_Seek_BufferReuse(t *testing.T) { + recorder := &readSeekRecorder{rs: bytes.NewReader(make([]byte, 100))} + + rs := NewReadSeekerSize(recorder, 20) + if len(rs.buf) != 20 { + t.Fatal("the buffer size was changed and the validity of this test has become unknown") + } + + // Seek to some random position. + p, err := rs.Seek(10, io.SeekStart) + if err != nil { + t.Fatalf("seek error: %v", err) + } + if p != 10 { + t.Fatalf("seek position mismatch: expected %d, got %d", 10, p) + } + recorder.assertSeeks(t, []seekRecord{{offset: 10, whence: io.SeekStart}}) + + // Read some bytes to fill the internal buffer. + // Buffer should span [10, 30). + n, err := rs.Read(make([]byte, 10)) + if err != nil { + t.Fatalf("read error: %v", err) + } + if n != 10 { + t.Fatalf("mismatch in # of bytes read: expected: %d, got %d", 10, n) + } + if rs.r != 10 { + t.Fatalf("buffer read position mismatch: expected: %d, got %d", 10, rs.r) + } + if rs.w != 20 { + t.Fatalf("buffer write position mismatch: expected: %d, got %d", 20, rs.w) + } + recorder.assertReads(t, []readRecord{{requested: 20}}) + + // Seek to an earlier position within the buffer. + p, err = rs.Seek(-10, io.SeekCurrent) + if err != nil { + t.Fatalf("seek error: %v", err) + } + if p != 10 { + t.Fatalf("seek position mismatch: expected %d, got %d", 10, p) + } + recorder.assertSeeks(t, nil) // no seeks + + // Seek to a later position within the buffer. + p, err = rs.Seek(25, io.SeekStart) + if err != nil { + t.Fatalf("seek error: %v", err) + } + if p != 25 { + t.Fatalf("seek position mismatch: expected %d, got %d", 25, p) + } + recorder.assertSeeks(t, nil) // no seeks + + // Read more than is within the buffer. + n, err = rs.Read(make([]byte, 10)) + if err != nil { + t.Fatalf("read error: %v", err) + } + if n != 5 { // should only have returned the bytes in the buffer. + t.Fatalf("mismatch in # of bytes read: expected: %d, got %d", 5, n) + } + if rs.r != 20 { + t.Fatalf("buffer read position mismatch: expected: %d, got %d", 20, rs.r) + } + if rs.w != 20 { + t.Fatalf("buffer write position mismatch: expected: %d, got %d", 20, rs.w) + } + recorder.assertReads(t, nil) // no reads + + // Read again. This will fill a new buffer. + n, err = rs.Read(make([]byte, 10)) + if err != nil { + t.Fatalf("read error: %v", err) + } + if n != 10 { + t.Fatalf("mismatch in # of bytes read: expected: %d, got %d", 10, n) + } + if rs.pos != 30 { + t.Fatalf("buffer start position mismatch: expected: %d, got %d", 30, rs.pos) + } + if rs.r != 10 { + t.Fatalf("buffer read position mismatch: expected: %d, got %d", 10, rs.r) + } + if rs.w != 20 { + t.Fatalf("buffer write position mismatch: expected: %d, got %d", 20, rs.w) + } + recorder.assertReads(t, []readRecord{{requested: 20}}) +} + +func TestReadSeeker_Seek(t *testing.T) { + type test struct { + seekTo int64 + bytes []byte + readErr error + } + + // The test is going to read 2 bytes at specified seek positions + // out of a buffer of size 100. + tests := []test{ + // Start + {seekTo: 0, bytes: []byte{0, 1}}, + + // Overlapping positions within a buffer. + {seekTo: 10, bytes: []byte{10, 11}}, + {seekTo: 20, bytes: []byte{20, 21}}, + + // End + {seekTo: 99, bytes: []byte{99}, readErr: nil}, + {seekTo: 100, bytes: []byte{}, readErr: io.EOF}, + } + + // Test seeking to one position, then another. + for _, test1 := range tests { + for _, test2 := range tests { + t.Run(fmt.Sprintf("seek_to_%d_and_%d", test1.seekTo, test2.seekTo), func(t *testing.T) { + bs := make([]byte, 100) + for i := range bs { + bs[i] = byte(i) + } + recorder := &readSeekRecorder{rs: bytes.NewReader(bs)} + + rs := NewReadSeekerSize(recorder, 20) + if len(rs.buf) != 20 { + t.Fatal("the buffer size was changed and the validity of this test has become unknown") + } + + // Seek to the first position. + p, err := rs.Seek(test1.seekTo, io.SeekStart) + if err != nil { + t.Fatalf("seek error: %v", err) + } + if p != test1.seekTo { + t.Fatalf("seek position mismatch: expected %d, got %d", test1.seekTo, p) + } + + // Read to trigger a buffer read. + _, _ = rs.Read([]byte{0x00}) + if err != nil && err != io.EOF { + t.Fatalf("seek error: %v", err) + } + + // Seek to the second position. + p, err = rs.Seek(test2.seekTo, io.SeekStart) + if err != nil { + t.Fatalf("seek error: %v", err) + } + if p != test2.seekTo { + t.Fatalf("seek position mismatch: expected %d, got %d", test2.seekTo, p) + } + + // Check a subsequent read works as expected. + got := make([]byte, 2) + n, err := rs.Read(got) + if err != test2.readErr { + t.Fatalf("error mismatch: expected %v, got %v", test2.readErr, err) + } + got = got[:n] + if !reflect.DeepEqual(test2.bytes, got) { + t.Fatalf("mismatch bytes returned by Read(): expected %#v, got %#v", test2.bytes, got) + } + }) + } + } +} + +func Test_Read_BigBuffer(t *testing.T) { + recorder := &readSeekRecorder{rs: bytes.NewReader(make([]byte, 100))} + + rs := NewReadSeekerSize(recorder, 20) + if len(rs.buf) != 20 { + t.Fatal("the buffer size was changed and the validity of this test has become unknown") + } + + got := make([]byte, 50) + n, err := rs.Read(got) + if err != nil { + t.Fatalf("read error: %v", err) + } + if n != 50 { + t.Fatalf("mismatch in # of bytes read: expected: %d, got %d", 50, n) + } + recorder.assertReads(t, []readRecord{{requested: 50}}) + + p, err := rs.Seek(0, io.SeekCurrent) + if err != nil { + t.Fatalf("read error: %v", err) + } + if p != 50 { + t.Fatalf("seek position mismatch: expected %d, got %d", 50, p) + } +} + +type readRecord struct { + requested int // number of bytes requested +} + +type seekRecord struct { + offset int64 + whence int +} + +type readSeekRecorder struct { + rs io.ReadSeeker + reads []readRecord + seeks []seekRecord +} + +func (r *readSeekRecorder) Read(p []byte) (n int, err error) { + r.reads = append(r.reads, readRecord{requested: len(p)}) + return r.rs.Read(p) +} + +func (r *readSeekRecorder) Seek(offset int64, whence int) (int64, error) { + r.seeks = append(r.seeks, seekRecord{offset: offset, whence: whence}) + return r.rs.Seek(offset, whence) +} + +func (r *readSeekRecorder) assertReads(t *testing.T, expected []readRecord) { + t.Helper() + + if !reflect.DeepEqual(expected, r.reads) { + t.Fatalf("read mismatch; expected %#v, got %#v", expected, r.reads) + } + // Clear reads + r.reads = nil +} + +func (r *readSeekRecorder) assertSeeks(t *testing.T, expected []seekRecord) { + t.Helper() + + if !reflect.DeepEqual(expected, r.seeks) { + t.Fatalf("seek mismatch; expected %#v, got %#v", expected, r.seeks) + } + // Clear seeks + r.seeks = nil +} From 4771aa96047e98bcc72c1821ec1448f6bd7618d7 Mon Sep 17 00:00:00 2001 From: Mark Kremer Date: Wed, 7 Aug 2024 20:58:31 +0200 Subject: [PATCH 3/4] Rewrite ReadSeeker tests --- internal/bufseekio/readseeker.go | 18 +- internal/bufseekio/readseeker_test.go | 403 +++++++++++--------------- 2 files changed, 185 insertions(+), 236 deletions(-) diff --git a/internal/bufseekio/readseeker.go b/internal/bufseekio/readseeker.go index cf9ce2b..9678eda 100644 --- a/internal/bufseekio/readseeker.go +++ b/internal/bufseekio/readseeker.go @@ -69,7 +69,7 @@ func (b *ReadSeeker) readErr() error { func (b *ReadSeeker) Read(p []byte) (n int, err error) { n = len(p) if n == 0 { - if b.Buffered() > 0 { + if b.buffered() > 0 { return 0, nil } return 0, b.readErr() @@ -110,8 +110,8 @@ func (b *ReadSeeker) Read(p []byte) (n int, err error) { return n, nil } -// Buffered returns the number of bytes that can be read from the current buffer. -func (b *ReadSeeker) Buffered() int { return b.w - b.r } +// buffered returns the number of bytes that can be read from the current buffer. +func (b *ReadSeeker) buffered() int { return b.w - b.r } func (b *ReadSeeker) Seek(offset int64, whence int) (int64, error) { // The stream.Seek() implementation makes heavy use of seeking with offset 0 @@ -125,17 +125,17 @@ func (b *ReadSeeker) Seek(offset int64, whence int) (int64, error) { return b.seek(offset, whence) } // Calculate the absolute offset. - absOffset := offset + abs := offset if whence == io.SeekCurrent { - absOffset += b.position() + abs += b.position() } // Check if the offset is within buf. - if absOffset >= b.pos && absOffset < b.pos+int64(b.w) { - b.r = int(absOffset - b.pos) - return absOffset, nil + if abs >= b.pos && abs < b.pos+int64(b.w) { + b.r = int(abs - b.pos) + return abs, nil } - return b.seek(offset, whence) + return b.seek(abs, io.SeekStart) } func (b *ReadSeeker) seek(offset int64, whence int) (int64, error) { diff --git a/internal/bufseekio/readseeker_test.go b/internal/bufseekio/readseeker_test.go index 951686e..cf47538 100644 --- a/internal/bufseekio/readseeker_test.go +++ b/internal/bufseekio/readseeker_test.go @@ -2,277 +2,235 @@ package bufseekio import ( "bytes" - "fmt" + "errors" "io" "reflect" "testing" ) -func TestReadSeeker_Seek_SeekCurrent(t *testing.T) { - recorder := &readSeekRecorder{rs: bytes.NewReader(make([]byte, 100))} +func TestNewReadSeekerSize(t *testing.T) { + buf := bytes.NewReader(make([]byte, 100)) - rs := NewReadSeekerSize(recorder, 20) - if len(rs.buf) != 20 { - t.Fatal("the buffer size was changed and the validity of this test has become unknown") + // Test custom buffer size. + if rs := NewReadSeekerSize(buf, 20); len(rs.buf) != 20 { + t.Fatalf("want %d got %d", 20, len(rs.buf)) } - // Seek forwards. - p, err := rs.Seek(10, io.SeekCurrent) - if err != nil { - t.Fatalf("seek error: %v", err) + // Test too small buffer size. + if rs := NewReadSeekerSize(buf, 1); len(rs.buf) != minReadBufferSize { + t.Fatalf("want %d got %d", minReadBufferSize, len(rs.buf)) } - if p != 10 { - t.Fatalf("seek position mismatch: expected %d, got %d", 10, p) - } - recorder.assertSeeks(t, []seekRecord{{offset: 10, whence: io.SeekCurrent}}) - // Get position without moving. - p, err = rs.Seek(0, io.SeekCurrent) - if err != nil { - t.Fatalf("seek error: %v", err) - } - if p != 10 { - t.Fatalf("seek position mismatch: expected %d, got %d", 10, p) + // Test reuse existing ReadSeeker. + rs := NewReadSeekerSize(buf, 20) + if rs2 := NewReadSeekerSize(rs, 5); rs != rs2 { + t.Fatal("expected ReadSeeker to be reused but got a different ReadSeeker") } - recorder.assertSeeks(t, nil) +} - // Move backwards. - p, err = rs.Seek(-5, io.SeekCurrent) - if err != nil { - t.Fatalf("seek error: %v", err) - } - if p != 5 { - t.Fatalf("seek position mismatch: expected %d, got %d", 5, p) +func TestNewReadSeeker(t *testing.T) { + buf := bytes.NewReader(make([]byte, 100)) + if rs := NewReadSeeker(buf); len(rs.buf) != defaultBufSize { + t.Fatalf("want %d got %d", defaultBufSize, len(rs.buf)) } - recorder.assertSeeks(t, []seekRecord{{offset: -5, whence: io.SeekCurrent}}) } -func TestReadSeeker_Seek_SeekEnd(t *testing.T) { - recorder := &readSeekRecorder{rs: bytes.NewReader(make([]byte, 100))} - - rs := NewReadSeekerSize(recorder, 20) +func TestReadSeeker_Read(t *testing.T) { + data := make([]byte, 100) + for i := range data { + data[i] = byte(i) + } + rs := NewReadSeekerSize(bytes.NewReader(data), 20) if len(rs.buf) != 20 { t.Fatal("the buffer size was changed and the validity of this test has become unknown") } - // Seek from end. - p, err := rs.Seek(-10, io.SeekEnd) - if err != nil { - t.Fatalf("seek error: %v", err) + // Test small read. + got := make([]byte, 5) + if n, err := rs.Read(got); err != nil || n != 5 || !reflect.DeepEqual(got, []byte{0, 1, 2, 3, 4}) { + t.Fatalf("want #bytes %d got %d, want buffer %v got %v, err=%v", 5, n, []byte{0, 1, 2, 3, 4}, got, err) } - if p != 90 { - t.Fatalf("seek position mismatch: expected %d, got %d", 90, p) + if p, err := rs.Seek(0, io.SeekCurrent); err != nil || p != 5 { + t.Fatalf("want %d got %d, err=%v", 5, p, err) } - recorder.assertSeeks(t, []seekRecord{{offset: -10, whence: io.SeekEnd}}) - // Seek from end again. - p, err = rs.Seek(-10, io.SeekEnd) - if err != nil { - t.Fatalf("seek error: %v", err) + // Test big read with initially filled buffer. + got = make([]byte, 25) + if n, err := rs.Read(got); err != nil || n != 15 || !reflect.DeepEqual(got, []byte{5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}) { + t.Fatalf("want n read %d got %d, want buffer %v got %v, err=%v", 15, n, []byte{5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, got, err) } - if p != 90 { - t.Fatalf("seek position mismatch: expected %d, got %d", 90, p) + if p, err := rs.Seek(0, io.SeekCurrent); err != nil || p != 20 { + t.Fatalf("want %d got %d, err=%v", 20, p, err) + } + + // Test big read with initially empty buffer. + if n, err := rs.Read(got); err != nil || n != 25 || !reflect.DeepEqual(got, []byte{20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44}) { + t.Fatalf("want n read %d got %d, want buffer %v got %v, err=%v", 5, n, []byte{20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44}, got, err) + } + if p, err := rs.Seek(0, io.SeekCurrent); err != nil || p != 45 { + t.Fatalf("want %d got %d, err=%v", 45, p, err) } - // It will always seek again because it only keeps track of the position from the start. - recorder.assertSeeks(t, []seekRecord{{offset: -10, whence: io.SeekEnd}}) -} -func TestReadSeeker_Seek_BufferReuse(t *testing.T) { - recorder := &readSeekRecorder{rs: bytes.NewReader(make([]byte, 100))} + // Test EOF. + if p, err := rs.Seek(98, io.SeekStart); err != nil || p != 98 { + t.Fatalf("want %d got %d, err=%v", 98, p, err) + } + got = make([]byte, 5) + if n, err := rs.Read(got); err != nil || n != 2 || !reflect.DeepEqual(got, []byte{98, 99, 0, 0, 0}) { + t.Fatalf("want #bytes %d got %d, want buffer %v got %v, err=%v", 2, n, []byte{98, 99, 0, 0, 0}, got, err) + } + if n, err := rs.Read(got); err != io.EOF || n != 0 { + t.Fatalf("want #bytes %d got %d, err=%v", 0, n, err) + } - rs := NewReadSeekerSize(recorder, 20) + // Test source that returns bytes and an error at the same time. + rs = NewReadSeekerSize(&readAndError{bytes: []byte{2, 3, 5}}, 20) if len(rs.buf) != 20 { t.Fatal("the buffer size was changed and the validity of this test has become unknown") } - - // Seek to some random position. - p, err := rs.Seek(10, io.SeekStart) - if err != nil { - t.Fatalf("seek error: %v", err) + got = make([]byte, 5) + if n, err := rs.Read(got); err != nil || n != 3 || !reflect.DeepEqual(got, []byte{2, 3, 5, 0, 0}) { + t.Fatalf("want #bytes %d got %d, want buffer %v got %v, err=%v", 3, n, []byte{2, 3, 5, 0, 0}, got, err) } - if p != 10 { - t.Fatalf("seek position mismatch: expected %d, got %d", 10, p) + if n, err := rs.Read(got); err != expectedErr || n != 0 { + t.Fatalf("want #bytes %d got %d, want error %v, got %v", 0, n, expectedErr, err) } - recorder.assertSeeks(t, []seekRecord{{offset: 10, whence: io.SeekStart}}) - // Read some bytes to fill the internal buffer. - // Buffer should span [10, 30). - n, err := rs.Read(make([]byte, 10)) - if err != nil { - t.Fatalf("read error: %v", err) + // Test read nothing with an empty buffer and a queued error. + rs = NewReadSeekerSize(&readAndError{bytes: []byte{2, 3, 5}}, 20) + if len(rs.buf) != 20 { + t.Fatal("the buffer size was changed and the validity of this test has become unknown") } - if n != 10 { - t.Fatalf("mismatch in # of bytes read: expected: %d, got %d", 10, n) + got = make([]byte, 3) + if n, err := rs.Read(got); err != nil || n != 3 || !reflect.DeepEqual(got, []byte{2, 3, 5}) { + t.Fatalf("want #bytes %d got %d, want buffer %v got %v, err=%v", 3, n, []byte{2, 3, 5}, got, err) } - if rs.r != 10 { - t.Fatalf("buffer read position mismatch: expected: %d, got %d", 10, rs.r) + if n, err := rs.Read(nil); err != expectedErr || n != 0 { + t.Fatalf("want #bytes %d got %d, want error %v, got %v", 0, n, expectedErr, err) } - if rs.w != 20 { - t.Fatalf("buffer write position mismatch: expected: %d, got %d", 20, rs.w) + if n, err := rs.Read(nil); err != nil || n != 0 { + t.Fatalf("want #bytes %d got %d, err=%v", 5, n, err) } - recorder.assertReads(t, []readRecord{{requested: 20}}) - // Seek to an earlier position within the buffer. - p, err = rs.Seek(-10, io.SeekCurrent) - if err != nil { - t.Fatalf("seek error: %v", err) + // Test read nothing with a non-empty buffer and a queued error. + rs = NewReadSeekerSize(&readAndError{bytes: []byte{2, 3, 5}}, 20) + if len(rs.buf) != 20 { + t.Fatal("the buffer size was changed and the validity of this test has become unknown") } - if p != 10 { - t.Fatalf("seek position mismatch: expected %d, got %d", 10, p) + got = make([]byte, 1) + if n, err := rs.Read(got); err != nil || n != 1 || !reflect.DeepEqual(got, []byte{2}) { + t.Fatalf("want #bytes %d got %d, want buffer %v got %v, err=%v", 1, n, []byte{}, got, err) } - recorder.assertSeeks(t, nil) // no seeks - - // Seek to a later position within the buffer. - p, err = rs.Seek(25, io.SeekStart) - if err != nil { - t.Fatalf("seek error: %v", err) + if n, err := rs.Read(nil); err != nil || n != 0 { + t.Fatalf("want #bytes %d got %d, err=%v", 0, n, err) } - if p != 25 { - t.Fatalf("seek position mismatch: expected %d, got %d", 25, p) +} + +var expectedErr = errors.New("expected error") + +type readAndError struct { + bytes []byte +} + +func (r *readAndError) Read(p []byte) (n int, err error) { + for i, b := range r.bytes { + p[i] = b } - recorder.assertSeeks(t, nil) // no seeks + return len(r.bytes), expectedErr +} + +func (r *readAndError) Seek(offset int64, whence int) (int64, error) { + panic("not implemented") +} - // Read more than is within the buffer. - n, err = rs.Read(make([]byte, 10)) - if err != nil { - t.Fatalf("read error: %v", err) +func TestReadSeeker_Seek(t *testing.T) { + data := make([]byte, 100) + for i := range data { + data[i] = byte(i) } - if n != 5 { // should only have returned the bytes in the buffer. - t.Fatalf("mismatch in # of bytes read: expected: %d, got %d", 5, n) + r := &seekRecorder{rs: bytes.NewReader(data)} + rs := NewReadSeekerSize(r, 20) + if len(rs.buf) != 20 { + t.Fatal("the buffer size was changed and the validity of this test has become unknown") } - if rs.r != 20 { - t.Fatalf("buffer read position mismatch: expected: %d, got %d", 20, rs.r) + + got := make([]byte, 5) + + // Test with io.SeekStart + if p, err := rs.Seek(10, io.SeekStart); err != nil || p != 10 { + t.Fatalf("want %d got %d, err=%v", 10, p, err) + } + r.assertSeeked(t, []seekRecord{{10, io.SeekStart}}) + if n, err := rs.Read(got); err != nil || n != 5 || !reflect.DeepEqual(got, []byte{10, 11, 12, 13, 14}) { + t.Fatalf("want n read %d got %d, want buffer %v got %v, err=%v", 5, n, []byte{10, 11, 12, 13, 14}, got, err) } - if rs.w != 20 { - t.Fatalf("buffer write position mismatch: expected: %d, got %d", 20, rs.w) + if p, err := rs.Seek(0, io.SeekCurrent); err != nil || p != 15 { + t.Fatalf("want %d got %d, err=%v", 15, p, err) } - recorder.assertReads(t, nil) // no reads + r.assertSeeked(t, nil) - // Read again. This will fill a new buffer. - n, err = rs.Read(make([]byte, 10)) - if err != nil { - t.Fatalf("read error: %v", err) + // Test with io.SeekCurrent and positive offset within buffer. + if p, err := rs.Seek(5, io.SeekCurrent); err != nil || p != 20 { + t.Fatalf("want %d got %d, err=%v", 20, p, err) } - if n != 10 { - t.Fatalf("mismatch in # of bytes read: expected: %d, got %d", 10, n) + r.assertSeeked(t, nil) + if n, err := rs.Read(got); err != nil || n != 5 || !reflect.DeepEqual(got, []byte{20, 21, 22, 23, 24}) { + t.Fatalf("want n read %d got %d, want buffer %v got %v, err=%v", 5, n, []byte{20, 21, 22, 23, 24}, got, err) } - if rs.pos != 30 { - t.Fatalf("buffer start position mismatch: expected: %d, got %d", 30, rs.pos) + if p, err := rs.Seek(0, io.SeekCurrent); err != nil || p != 25 { + t.Fatalf("want %d got %d, err=%v", 25, p, err) } - if rs.r != 10 { - t.Fatalf("buffer read position mismatch: expected: %d, got %d", 10, rs.r) + + // Test with io.SeekCurrent and negative offset within buffer. + if p, err := rs.Seek(-10, io.SeekCurrent); err != nil || p != 15 { + t.Fatalf("want %d got %d, err=%v", 15, p, err) } - if rs.w != 20 { - t.Fatalf("buffer write position mismatch: expected: %d, got %d", 20, rs.w) + r.assertSeeked(t, nil) + if n, err := rs.Read(got); err != nil || n != 5 || !reflect.DeepEqual(got, []byte{15, 16, 17, 18, 19}) { + t.Fatalf("want n read %d got %d, want buffer %v got %v, err=%v", 5, n, []byte{15, 16, 17, 18, 19}, got, err) } - recorder.assertReads(t, []readRecord{{requested: 20}}) -} - -func TestReadSeeker_Seek(t *testing.T) { - type test struct { - seekTo int64 - bytes []byte - readErr error - } - - // The test is going to read 2 bytes at specified seek positions - // out of a buffer of size 100. - tests := []test{ - // Start - {seekTo: 0, bytes: []byte{0, 1}}, - - // Overlapping positions within a buffer. - {seekTo: 10, bytes: []byte{10, 11}}, - {seekTo: 20, bytes: []byte{20, 21}}, - - // End - {seekTo: 99, bytes: []byte{99}, readErr: nil}, - {seekTo: 100, bytes: []byte{}, readErr: io.EOF}, - } - - // Test seeking to one position, then another. - for _, test1 := range tests { - for _, test2 := range tests { - t.Run(fmt.Sprintf("seek_to_%d_and_%d", test1.seekTo, test2.seekTo), func(t *testing.T) { - bs := make([]byte, 100) - for i := range bs { - bs[i] = byte(i) - } - recorder := &readSeekRecorder{rs: bytes.NewReader(bs)} - - rs := NewReadSeekerSize(recorder, 20) - if len(rs.buf) != 20 { - t.Fatal("the buffer size was changed and the validity of this test has become unknown") - } - - // Seek to the first position. - p, err := rs.Seek(test1.seekTo, io.SeekStart) - if err != nil { - t.Fatalf("seek error: %v", err) - } - if p != test1.seekTo { - t.Fatalf("seek position mismatch: expected %d, got %d", test1.seekTo, p) - } - - // Read to trigger a buffer read. - _, _ = rs.Read([]byte{0x00}) - if err != nil && err != io.EOF { - t.Fatalf("seek error: %v", err) - } - - // Seek to the second position. - p, err = rs.Seek(test2.seekTo, io.SeekStart) - if err != nil { - t.Fatalf("seek error: %v", err) - } - if p != test2.seekTo { - t.Fatalf("seek position mismatch: expected %d, got %d", test2.seekTo, p) - } - - // Check a subsequent read works as expected. - got := make([]byte, 2) - n, err := rs.Read(got) - if err != test2.readErr { - t.Fatalf("error mismatch: expected %v, got %v", test2.readErr, err) - } - got = got[:n] - if !reflect.DeepEqual(test2.bytes, got) { - t.Fatalf("mismatch bytes returned by Read(): expected %#v, got %#v", test2.bytes, got) - } - }) - } + if p, err := rs.Seek(0, io.SeekCurrent); err != nil || p != 20 { + t.Fatalf("want %d got %d, err=%v", 20, p, err) } -} -func Test_Read_BigBuffer(t *testing.T) { - recorder := &readSeekRecorder{rs: bytes.NewReader(make([]byte, 100))} - - rs := NewReadSeekerSize(recorder, 20) - if len(rs.buf) != 20 { - t.Fatal("the buffer size was changed and the validity of this test has become unknown") + // Test with io.SeekCurrent and positive offset outside buffer. + if p, err := rs.Seek(30, io.SeekCurrent); err != nil || p != 50 { + t.Fatalf("want %d got %d, err=%v", 50, p, err) } - - got := make([]byte, 50) - n, err := rs.Read(got) - if err != nil { - t.Fatalf("read error: %v", err) + r.assertSeeked(t, []seekRecord{{50, io.SeekStart}}) + if n, err := rs.Read(got); err != nil || n != 5 || !reflect.DeepEqual(got, []byte{50, 51, 52, 53, 54}) { + t.Fatalf("want n read %d got %d, want buffer %v got %v, err=%v", 5, n, []byte{50, 51, 52, 53, 54}, got, err) } - if n != 50 { - t.Fatalf("mismatch in # of bytes read: expected: %d, got %d", 50, n) + if p, err := rs.Seek(0, io.SeekCurrent); err != nil || p != 55 { + t.Fatalf("want %d got %d, err=%v", 55, p, err) } - recorder.assertReads(t, []readRecord{{requested: 50}}) - p, err := rs.Seek(0, io.SeekCurrent) - if err != nil { - t.Fatalf("read error: %v", err) + // Test seek with io.SeekEnd within buffer. + if p, err := rs.Seek(-45, io.SeekEnd); err != nil || p != 55 { + t.Fatalf("want %d got %d, err=%v", 55, p, err) } - if p != 50 { - t.Fatalf("seek position mismatch: expected %d, got %d", 50, p) + r.assertSeeked(t, []seekRecord{{-45, io.SeekEnd}}) + if n, err := rs.Read(got); err != nil || n != 5 || !reflect.DeepEqual(got, []byte{55, 56, 57, 58, 59}) { + t.Fatalf("want n read %d got %d, want buffer %v got %v, err=%v", 5, n, []byte{55, 56, 57, 58, 59}, got, err) + } + if p, err := rs.Seek(0, io.SeekCurrent); err != nil || p != 60 { + t.Fatalf("want %d got %d, err=%v", 60, p, err) } -} -type readRecord struct { - requested int // number of bytes requested + // Test seek with error. + if _, err := rs.Seek(-100, io.SeekStart); err == nil || err.Error() != "bytes.Reader.Seek: negative position" { + t.Fatalf("want error 'bytes.Reader.Seek: negative position' got %v", err) + } + r.assertSeeked(t, []seekRecord{{-100, io.SeekStart}}) + + // Test seek after error. + if p, err := rs.Seek(10, io.SeekStart); err != nil || p != 10 { + t.Fatalf("want %d got %d, err=%v", 10, p, err) + } + r.assertSeeked(t, []seekRecord{{10, io.SeekStart}}) + if n, err := rs.Read(got); err != nil || n != 5 || !reflect.DeepEqual(got, []byte{10, 11, 12, 13, 14}) { + t.Fatalf("want n read %d got %d, want buffer %v got %v, err=%v", 5, n, []byte{10, 11, 12, 13, 14}, got, err) + } } type seekRecord struct { @@ -280,38 +238,29 @@ type seekRecord struct { whence int } -type readSeekRecorder struct { +type seekRecorder struct { rs io.ReadSeeker - reads []readRecord seeks []seekRecord } -func (r *readSeekRecorder) Read(p []byte) (n int, err error) { - r.reads = append(r.reads, readRecord{requested: len(p)}) +func (r *seekRecorder) Read(p []byte) (n int, err error) { return r.rs.Read(p) } -func (r *readSeekRecorder) Seek(offset int64, whence int) (int64, error) { +func (r *seekRecorder) Seek(offset int64, whence int) (int64, error) { r.seeks = append(r.seeks, seekRecord{offset: offset, whence: whence}) return r.rs.Seek(offset, whence) } -func (r *readSeekRecorder) assertReads(t *testing.T, expected []readRecord) { - t.Helper() - - if !reflect.DeepEqual(expected, r.reads) { - t.Fatalf("read mismatch; expected %#v, got %#v", expected, r.reads) - } - // Clear reads - r.reads = nil -} - -func (r *readSeekRecorder) assertSeeks(t *testing.T, expected []seekRecord) { +func (r *seekRecorder) assertSeeked(t *testing.T, expected []seekRecord) { t.Helper() if !reflect.DeepEqual(expected, r.seeks) { t.Fatalf("seek mismatch; expected %#v, got %#v", expected, r.seeks) } - // Clear seeks + r.reset() +} + +func (r *seekRecorder) reset() { r.seeks = nil } From 19a9efb4f68fecaf12375248848f7cbe94677283 Mon Sep 17 00:00:00 2001 From: Mark Kremer Date: Wed, 7 Aug 2024 21:05:28 +0200 Subject: [PATCH 4/4] Fix error message inconsistencies --- internal/bufseekio/readseeker_test.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/internal/bufseekio/readseeker_test.go b/internal/bufseekio/readseeker_test.go index cf47538..06b162d 100644 --- a/internal/bufseekio/readseeker_test.go +++ b/internal/bufseekio/readseeker_test.go @@ -48,7 +48,7 @@ func TestReadSeeker_Read(t *testing.T) { // Test small read. got := make([]byte, 5) if n, err := rs.Read(got); err != nil || n != 5 || !reflect.DeepEqual(got, []byte{0, 1, 2, 3, 4}) { - t.Fatalf("want #bytes %d got %d, want buffer %v got %v, err=%v", 5, n, []byte{0, 1, 2, 3, 4}, got, err) + t.Fatalf("want n read %d got %d, want buffer %v got %v, err=%v", 5, n, []byte{0, 1, 2, 3, 4}, got, err) } if p, err := rs.Seek(0, io.SeekCurrent); err != nil || p != 5 { t.Fatalf("want %d got %d, err=%v", 5, p, err) @@ -65,7 +65,7 @@ func TestReadSeeker_Read(t *testing.T) { // Test big read with initially empty buffer. if n, err := rs.Read(got); err != nil || n != 25 || !reflect.DeepEqual(got, []byte{20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44}) { - t.Fatalf("want n read %d got %d, want buffer %v got %v, err=%v", 5, n, []byte{20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44}, got, err) + t.Fatalf("want n read %d got %d, want buffer %v got %v, err=%v", 25, n, []byte{20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44}, got, err) } if p, err := rs.Seek(0, io.SeekCurrent); err != nil || p != 45 { t.Fatalf("want %d got %d, err=%v", 45, p, err) @@ -77,10 +77,10 @@ func TestReadSeeker_Read(t *testing.T) { } got = make([]byte, 5) if n, err := rs.Read(got); err != nil || n != 2 || !reflect.DeepEqual(got, []byte{98, 99, 0, 0, 0}) { - t.Fatalf("want #bytes %d got %d, want buffer %v got %v, err=%v", 2, n, []byte{98, 99, 0, 0, 0}, got, err) + t.Fatalf("want n read %d got %d, want buffer %v got %v, err=%v", 2, n, []byte{98, 99, 0, 0, 0}, got, err) } if n, err := rs.Read(got); err != io.EOF || n != 0 { - t.Fatalf("want #bytes %d got %d, err=%v", 0, n, err) + t.Fatalf("want n read %d got %d, err=%v", 0, n, err) } // Test source that returns bytes and an error at the same time. @@ -90,10 +90,10 @@ func TestReadSeeker_Read(t *testing.T) { } got = make([]byte, 5) if n, err := rs.Read(got); err != nil || n != 3 || !reflect.DeepEqual(got, []byte{2, 3, 5, 0, 0}) { - t.Fatalf("want #bytes %d got %d, want buffer %v got %v, err=%v", 3, n, []byte{2, 3, 5, 0, 0}, got, err) + t.Fatalf("want n read %d got %d, want buffer %v got %v, err=%v", 3, n, []byte{2, 3, 5, 0, 0}, got, err) } if n, err := rs.Read(got); err != expectedErr || n != 0 { - t.Fatalf("want #bytes %d got %d, want error %v, got %v", 0, n, expectedErr, err) + t.Fatalf("want n read %d got %d, want error %v, got %v", 0, n, expectedErr, err) } // Test read nothing with an empty buffer and a queued error. @@ -103,13 +103,13 @@ func TestReadSeeker_Read(t *testing.T) { } got = make([]byte, 3) if n, err := rs.Read(got); err != nil || n != 3 || !reflect.DeepEqual(got, []byte{2, 3, 5}) { - t.Fatalf("want #bytes %d got %d, want buffer %v got %v, err=%v", 3, n, []byte{2, 3, 5}, got, err) + t.Fatalf("want n read %d got %d, want buffer %v got %v, err=%v", 3, n, []byte{2, 3, 5}, got, err) } if n, err := rs.Read(nil); err != expectedErr || n != 0 { - t.Fatalf("want #bytes %d got %d, want error %v, got %v", 0, n, expectedErr, err) + t.Fatalf("want n read %d got %d, want error %v, got %v", 0, n, expectedErr, err) } if n, err := rs.Read(nil); err != nil || n != 0 { - t.Fatalf("want #bytes %d got %d, err=%v", 5, n, err) + t.Fatalf("want n read %d got %d, err=%v", 0, n, err) } // Test read nothing with a non-empty buffer and a queued error. @@ -119,10 +119,10 @@ func TestReadSeeker_Read(t *testing.T) { } got = make([]byte, 1) if n, err := rs.Read(got); err != nil || n != 1 || !reflect.DeepEqual(got, []byte{2}) { - t.Fatalf("want #bytes %d got %d, want buffer %v got %v, err=%v", 1, n, []byte{}, got, err) + t.Fatalf("want n read %d got %d, want buffer %v got %v, err=%v", 1, n, []byte{}, got, err) } if n, err := rs.Read(nil); err != nil || n != 0 { - t.Fatalf("want #bytes %d got %d, err=%v", 0, n, err) + t.Fatalf("want n read %d got %d, err=%v", 0, n, err) } }