Skip to content

Commit

Permalink
fix(PageBufferReader): should conform to io.Reader interface (dgraph-…
Browse files Browse the repository at this point in the history
…io#1935)

Fixes DGRAPHCORE-121

## Problem
The CI build was showing sporadic failures of TestPagebufferReader2 with
the error message: Received unexpected error: EOF
The result is sporadic because the test case relied on randomized
behavior. In particular, it would generate a read-buffer of some random
length, which could occasionally have a length of 0. When the length
is 0, we would encounter this error.

The cause is from the PageBufferReader having incorrect behavior for the
Read(p []byte) function: In particular, when p is empty, based on the
expected behavior of the Reader interface, this should return 0, nil. 
However, this is currently returning 0, EOF.

## Solution
I added a unit test to make sure we consider the empty buffer input in
every case (instead of just randomly every once in a while). This
doesn't actually provide a solution, but it makes sure that we don't end
up with a regression in this area.

At the point where we currently detect that we read 0 bytes, before just
returning 0, EOF, I add a check to make sure that the value p has
non-zero length.
  • Loading branch information
billprovince authored May 4, 2023
1 parent 1bbd08d commit f12f5e1
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 1 deletion.
2 changes: 1 addition & 1 deletion y/y.go
Original file line number Diff line number Diff line change
Expand Up @@ -508,7 +508,7 @@ func (r *PageBufferReader) Read(p []byte) (int, error) {
}
}

if read == 0 {
if read == 0 && len(p) > 0 {
return read, io.EOF
}

Expand Down
22 changes: 22 additions & 0 deletions y/y_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,28 @@ func TestPagebufferReader4(t *testing.T) {
require.Equal(t, n, 0)
}

// Test when reading into 0 length readBuffer
func TestPagebufferReader5(t *testing.T) {
b := NewPageBuffer(32)
readerWhenEmptyPageBuffer := b.NewReaderAt(0)

readBuffer := []byte{} // Intentionally empty readBuffer.
n, err := readerWhenEmptyPageBuffer.Read(readBuffer)
require.NoError(t, err, "reading into empty buffer should return no error")
require.Equal(t, 0, n, "read into empty buffer should return 0 bytes")

var wb [20]byte
rand.Read(wb[:])
n, err = b.Write(wb[:])
require.Equal(t, n, len(wb), "length of buffer and length written should be equal")
require.NoError(t, err, "unable to write bytes to buffer")

readerWhenNonEmptyPageBuffer := b.NewReaderAt(0)
n, err = readerWhenNonEmptyPageBuffer.Read(readBuffer)
require.NoError(t, err, "reading into empty buffer should return no error")
require.Equal(t, 0, n, "read into empty buffer should return 0 bytes")
}

func TestSizeVarintForZero(t *testing.T) {
siz := sizeVarint(0)
require.Equal(t, 1, siz)
Expand Down

0 comments on commit f12f5e1

Please sign in to comment.