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

return an error when there are insufficient bytes #6

Closed
wants to merge 1 commit into from

Conversation

jacobheun
Copy link

This aims to resolve #5. This could probably be cleaner and I'm not positive I'm missing certain scenarios but I have tested this in several integrations in addition to the tests added for this. I'm happy to make updates and/or add some additional scenarios to the tests.

This updates the state to additionally keep track of how much data was requested (wants), how much data has been read and the total number of bytes that have gone through the states buffers. The idea, as mentioned in the comments for didOverread is to check if we're requesting bytes in excess of what is available, without breaking successive reads.

I've verified this integrated with:

@dominictarr
Copy link
Owner

I don't understand why "without breaking successive reads" is a problem. how does happen? Okay, you ask for N bytes, there is only two ways it doesn't give them. 1. the stream ends, and so there is no successive reads, 2. the read times out. There may be more data... but I think here the timeout should be considered an error (or you should increase the timeout, or disable the timeout)

Then you could simplify this because you don't have to think about "successive reads"

add tests for multireading and fix logic
simplify logic for overreading
@jacobheun
Copy link
Author

Thanks for outlining that, it cleared things up in my head. The "successive read" issue was me overcomplicating the logic when something like pull-block was used in the stream.

I've updated the code to simplify the logic and have re-run all my previous tests against the changes.

If we want to change the error message to just bytes that were read, I can remove the wants logic from this as well, something like only 1 of 10 bytes read.

@dominictarr
Copy link
Owner

while reviewing this I realized the same thing could be done much more simply, what do you think of: https://github.com/dominictarr/pull-reader/compare/error2?expand=1

@jacobheun jacobheun closed this Jul 6, 2018
@jacobheun jacobheun deleted the overreading branch July 6, 2018 23:04
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.

overreading
2 participants