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

make it possible to parse a varint at the end of a reader #3428

Merged
merged 1 commit into from May 26, 2022

Conversation

marten-seemann
Copy link
Member

An io.Reader can read into the buffer and return the io.EOF in the same call. In fact, that's how the quic.Stream is implemented.
In that case, we shouldn't return an error from quicvarint.Read, otherwise the caller won't be able to parse a varint sent just before a stream was closed.

@codecov
Copy link

codecov bot commented May 22, 2022

Codecov Report

Merging #3428 (d7622a2) into master (8185d1b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3428   +/-   ##
=======================================
  Coverage   85.44%   85.44%           
=======================================
  Files         136      136           
  Lines        9946     9948    +2     
=======================================
+ Hits         8498     8500    +2     
  Misses       1066     1066           
  Partials      382      382           
Impacted Files Coverage Δ
quicvarint/io.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8185d1b...d7622a2. Read the comment docs.

An io.Reader can read into the buffer and return the io.EOF in the same
call. In fact, that's how the quic.Stream is implemented.
In that case, we shouldn't return an error from quicvarint.Read,
otherwise the caller won't be able to parse a varint sent just before a
stream was closed.
Copy link
Collaborator

@MarcoPolo MarcoPolo 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!

@@ -31,7 +31,10 @@ func NewReader(r io.Reader) Reader {

func (r *byteReader) ReadByte() (byte, error) {
var b [1]byte
_, err := r.Reader.Read(b[:])
n, err := r.Reader.Read(b[:])
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is obvious and good as is, but as a general note ReadAtLeast follows the same semantics here (not suggesting we change this here).

@marten-seemann marten-seemann merged commit 9d5de12 into master May 26, 2022
@marten-seemann marten-seemann deleted the fix-varint-parsing-eof branch May 29, 2022 15:05
nmldiegues pushed a commit to chungthuang/quic-go that referenced this pull request Jun 6, 2022
An io.Reader can read into the buffer and return the io.EOF in the same
call. In fact, that's how the quic.Stream is implemented.
In that case, we shouldn't return an error from quicvarint.Read,
otherwise the caller won't be able to parse a varint sent just before a
stream was closed.
@MarcoPolo MarcoPolo mentioned this pull request Jul 7, 2022
41 tasks
sudarshan-reddy pushed a commit to sudarshan-reddy/quic-go that referenced this pull request Aug 9, 2022
An io.Reader can read into the buffer and return the io.EOF in the same
call. In fact, that's how the quic.Stream is implemented.
In that case, we shouldn't return an error from quicvarint.Read,
otherwise the caller won't be able to parse a varint sent just before a
stream was closed.
@ajnavarro ajnavarro mentioned this pull request Aug 24, 2022
72 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants