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

Retry in decoder if Read() returns 0 bytes read with nil error #387

Merged
merged 2 commits into from
Feb 20, 2023

Conversation

fxamacker
Copy link
Owner

@fxamacker fxamacker commented Jan 30, 2023

Previously, Decoder did not properly handle the scenario of Read implementations returning 0 bytes read with nil error.

Go docs for io.Reader discourage Read implementations from returning 0 bytes with nil error. And callers should treat this as if nothing happened.

Implementations of Read are discouraged from returning a zero byte count with a nil error, except when len(p) == 0. Callers should treat a return of 0 and nil as indicating that nothing happened; in particular it does not indicate EOF.

This PR:

  • Retries the call to Read if it returns (0, nil).
  • Refactors error handling.
  • Adds unit tests to reach 100% coverage of stream.go. 💯 🎉

Closes #385 - thanks @mixmasala for reporting this issue.

Previously, decoder did not properly handle the scenario of
Read() returning 0 bytes read with nil error.

Fix this by retrying the read on (0, nil).

While at it, also refactor the error handling code and add
more tests for 100% code coverage for stream.go.
@fxamacker fxamacker added the bug Something isn't working label Jan 30, 2023
decode.go Outdated
@@ -570,10 +579,10 @@ type decoder struct {
}

// value decodes CBOR data item into the value pointed to by v.
// If CBOR data item is invalid, error is returned and offset isn't changed.
// If CBOR data item is valid but fails to be decode into v for other reasons,
// If CBOR data item fails to be decode into v,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice work! Great to have 100% coverage in stream_test.go!

Didn't spot any problems in the code, but a comment has grammar error "fails to be decode into v".

Copy link
Contributor

@x448 x448 left a comment

Choose a reason for hiding this comment

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

👍

@fxamacker fxamacker merged commit f9e6291 into master Feb 20, 2023
@fxamacker fxamacker added this to the v2.5.0 milestone May 7, 2023
@fxamacker fxamacker deleted the fxamacker/fix-read-returns-0-and-nil-error branch June 18, 2023 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: Decoder returns no error on short read
2 participants