Skip to content

Conversation

qingyang-hu
Copy link
Collaborator

GODRIVER-3291

Summary

Refactor bson.valueReader with bufio.

Background & Motivation

@mongodb-drivers-pr-bot mongodb-drivers-pr-bot bot added the review-priority-low Low Priority PR for Review: within 3 business days label Aug 2, 2024
@qingyang-hu qingyang-hu marked this pull request as ready for review August 5, 2024 14:39
prestonvasquez
prestonvasquez previously approved these changes Aug 6, 2024
if err == nil {
err = io.EOF
}
buf := make([]byte, length)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional: The length is frequently static, which allows using static-size arrays or slices. To take advantage of that, consider changing the readBytes API to accept a []byte instead of a length.

E.g.

func (vr *valueReader) read(buf []byte) (int, error) { ... }

Then an example call from readi32 would look like:

func (vr *valueReader) readi32() (int32, error) {
	var buf [4]byte
	_, err := vr.read(buf[:])
	// ...

}

vr.pop()
_ = vr.pop()
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should handle this error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This line is inside an error-handling block. We don't need to overwrite the original error even when pop() returns an error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ErrEOD is a sentinel error, similar to io.EOF, so it's not an error handling block in the sense that "something has gone wrong". However, it does seem like the pop call here will never read from the underlying reader, so it should be safe to discard. Consider adding a comment that says the pop call here will never read from the underlying reader, so it's safe to discard the error.

}

vr.pop()
_ = vr.pop()
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should handle this error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This line is inside an error-handling block. We don't need to overwrite the original error even when pop() returns an error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

See comment above.

Copy link
Contributor

API Change Report

No changes found!

prestonvasquez
prestonvasquez previously approved these changes Aug 14, 2024
func (vr *valueReader) read(p []byte) error {
n, err := io.ReadFull(vr.r, p)
if errors.Is(err, io.ErrUnexpectedEOF) {
return io.EOF
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we convert an io.UnexpectedEOF to io.EOF here? io.UnexpectedEOF actually seems more appropriate considering this note from the io.EOF documentation:

Functions should return EOF only to signal a graceful end of input. If the EOF occurs unexpectedly in a structured data stream, the appropriate error is either ErrUnexpectedEOF or some other error giving more detail.

Some other valueReader methods should also be updated to return io.UnexpectedEOF if the end of the input was reached before

buf := make([]byte, length)
_, err = io.ReadFull(vr.r, buf)
if errors.Is(err, io.ErrUnexpectedEOF) {
return nil, io.EOF
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question as in read: Why do we convert an io.UnexpectedEOF to io.EOF here?

@blink1073 blink1073 added review-priority-normal Medium Priority PR for Review: within 1 business day and removed review-priority-low Low Priority PR for Review: within 3 business days labels Sep 12, 2024
Copy link
Collaborator

@matthewdale matthewdale left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@qingyang-hu qingyang-hu merged commit 32ff39b into mongodb:master Sep 17, 2024
30 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review-priority-normal Medium Priority PR for Review: within 1 business day

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants