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

Reader should handle 0-length reads better #13119

Closed
lilyball opened this issue Mar 24, 2014 · 1 comment
Closed

Reader should handle 0-length reads better #13119

lilyball opened this issue Mar 24, 2014 · 1 comment

Comments

@lilyball
Copy link
Contributor

Reader should be vetted to ensure it handles 0-length reads in a sane fashion. This is inspired by comments on #13049.

A few things need to be ensured:

  1. The documentation on Reader discourages returning 0 from read(). It should certainly be allowed, but implementors should be encouraged to avoid returning 0 if possible.
  2. The default methods on Reader should all behave sanely if a single call to read() returns 0.
  3. Less importantly, the default methods should handle a Reader that returns 0 forever. Practically speaking, this means that if the method employs a loop, it should also employ a counter to count how many 0-length read()s there have been in a row, and if it passes some threshold (TBD, but maybe something like 1000) then it should return an error. There should also be a new IoErrorKind defined for this case (Go uses one called ErrNoProgress, that seems a sensible name).
  4. Any clients of Reader in std and the associated libraries should also be checked to make sure they handle 0-length reads.

The simplest way to implement this may be to add another default method read_at_least() that takes a count of how many bytes it should ensure it has read. This method can then deal with counting 0-length reads and setting the proper error, and then everything that reads in a loop can use r.read_at_least(but, 1) so they don't have to deal with it.

@alexcrichton
Copy link
Member

I believe this was all fixed in #13127, so closing.

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

No branches or pull requests

2 participants