Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(PageBufferReader): PageBufferReader should conform to Reader interface #1935

Merged
merged 2 commits into from
May 4, 2023

Conversation

billprovince
Copy link
Contributor

Problem

Fixes DGRAPHCORE-121
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

First:
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.
Second:
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.

@CLAassistant
Copy link

CLAassistant commented May 4, 2023

CLA assistant check
All committers have signed the CLA.

@billprovince billprovince marked this pull request as ready for review May 4, 2023 17:40
Copy link
Contributor

@mangalaman93 mangalaman93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you for figuring this out.

@mangalaman93 mangalaman93 merged commit f12f5e1 into main May 4, 2023
@mangalaman93 mangalaman93 deleted the fixPageBufferReaderTry2 branch May 4, 2023 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants