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

EOF should be reported differently than returning 0 #2717

Open
polomsky opened this issue Jul 29, 2020 · 4 comments
Open

EOF should be reported differently than returning 0 #2717

polomsky opened this issue Jul 29, 2020 · 4 comments
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-io Module: tokio/io

Comments

@polomsky
Copy link

Is your feature request related to a problem? Please describe.
read function in AsyncReadExt forces user to check against zero which is legal value. EOF should be explicitly reported - similar to None in Option to do not allow to forget.

Describe the solution you'd like
Change result of read from io::Result<usize> to io::Result<Option<core::num::NonZeroUsize>> or to some enum (e.g. EOF,Length(core::num::NonZeroUsize)) - this will add no memory overhead with explicit EOF handling. This will also prevent using zero sized buffer for reading (this is sadly price of explicit checking and zero memory overhead) - for this Error would be reported.

Describe alternatives you've considered
I was thinking about using Option or enum with usize directly but I can not image real world scenario where actually using zero sized buffer for reading would be useful.

Additional context
It seems to me, that the checking if read returned 0 bytes and accept is as EOF looks similar to infamous null pointer problem. This is actually the purpose of structs like Option or Result to ensure that programmer must explicitly ignore things to make a problem.

@polomsky polomsky added A-tokio Area: The main tokio crate C-feature-request Category: A feature request. labels Jul 29, 2020
@Darksonn Darksonn added the M-io Module: tokio/io label Jul 29, 2020
@Diggsey
Copy link
Contributor

Diggsey commented Jul 31, 2020

The AsyncReadExt crate mirrors the Read trait from the stdlib. Switching to an enum would have the downside of departing from the libstd API.

@MikailBag
Copy link
Contributor

AFAIU read is low-level function: user should call it multiple times in loop. Higher-level stuff like read_exact or read_to_end is not affected by this proposal.
Are there any reasons for read returning 0 on EOF except "POSIX read and WINAPI ReadFile do that"? These are C functions, and C does not have Options.
RFC 517 "IO & OS reform" does not justify using usize instead of Option<NonZeroUsize> too, and I do not see other RFCs about std::io::Read::read.

Also: while AsyncReadExt would be different from std::io::Read, this difference can only cause compile error. It cannot lead to silent bugs, and it helps programmer make less bugs.

On the other hand, AsyncRead::read is used rarely (afaik), so benefit from such more-typed API is not large too.

@Darksonn
Copy link
Contributor

What do you think about #2716 in relation to this?

Are there any reasons for read returning 0 on EOF except "POSIX read and WINAPI ReadFile do that"? These are C functions, and C does not have Options.

Yes, for example there is the reason we used when initially designing the API, namely that the Rust standard library does this.

@polomsky
Copy link
Author

polomsky commented Aug 2, 2020

I understand the reason make it same as a std api. What actually caught my attention was a part of tutorial handling eof. And I simply could not understand why this specific case is not handled by different api design because Option is used quite extensively in rust. And I still believe that more strict api is better even it does not match standard library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-io Module: tokio/io
Projects
None yet
Development

No branches or pull requests

4 participants