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

Socket.readN yields more data than requested #957

Closed
tpolecat opened this Issue Oct 25, 2017 · 4 comments

Comments

Projects
None yet
3 participants
@tpolecat
Contributor

tpolecat commented Oct 25, 2017

Socket.readN(n) does not in fact read "exactly N bytes" but rather at least n bytes because the backing buffer is never shrunk; it is only embiggened as necessary. So readN will read as many bytes as in the biggest chunk requested thus far on that Socket. A quick fix is to change < to != here. but that may have an unacceptable performance impact so shrinking may need to be special-cased; or the readN0 method may need to be modified pull only the requested number of bytes.

@mpilquist

This comment has been minimized.

Show comment
Hide comment
@mpilquist

mpilquist Nov 1, 2017

Member

@pchlupacek Any thoughts on this?

Member

mpilquist commented Nov 1, 2017

@pchlupacek Any thoughts on this?

@pchlupacek

This comment has been minimized.

Show comment
Hide comment
@pchlupacek

pchlupacek Nov 1, 2017

Contributor

The fix shall be quite straightforward. Essentially we need to add another guard except the side of buffer to readChunk implementation. @tpolecat Any chance you can write test for this ?

Contributor

pchlupacek commented Nov 1, 2017

The fix shall be quite straightforward. Essentially we need to add another guard except the side of buffer to readChunk implementation. @tpolecat Any chance you can write test for this ?

@tpolecat

This comment has been minimized.

Show comment
Hide comment
@tpolecat

tpolecat Nov 1, 2017

Contributor

Yep, will do.

Contributor

tpolecat commented Nov 1, 2017

Yep, will do.

@pchlupacek

This comment has been minimized.

Show comment
Hide comment
@pchlupacek

pchlupacek Nov 1, 2017

Contributor
Contributor

pchlupacek commented Nov 1, 2017

@tpolecat tpolecat referenced this issue Nov 1, 2017

Merged

Test for 957 #966

@mpilquist mpilquist added this to the 0.10 milestone Jan 6, 2018

mpilquist added a commit to mpilquist/fs2 that referenced this issue Jan 8, 2018

@mpilquist mpilquist closed this in 583b014 Jan 8, 2018

mpilquist added a commit that referenced this issue Jan 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment