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

AsyncRead::poll_read() does not expose the EOF concept #1624

Open
djc opened this issue May 20, 2019 · 4 comments · Fixed by #1689
Open

AsyncRead::poll_read() does not expose the EOF concept #1624

djc opened this issue May 20, 2019 · 4 comments · Fixed by #1689
Labels
A-io Area: futures::io docs

Comments

@djc
Copy link

djc commented May 20, 2019

As of 85162dc, poll_read() is defined as:

fn poll_read(self: Pin<&mut Self>, cx: &mut Context<'_>, buf: &mut [u8])
    -> Poll<Result<usize>>;

In tokio-io (which seems to be the canonical definition for futures-0.1) it is defined as:

fn poll_read(&mut self, buf: &mut [u8]) -> Poll<usize, std_io::Error>;

In addition, the tokio-io documentation mentions:

/// * `Ok(Async::Ready(n))` means that `n` bytes of data was immediately read
///   and placed into the output buffer, where `n` == 0 implies that EOF has
///   been reached.

I don't much like the sentinel value of 0 for EOF, and I'm confused by the notion of EOF not being reflected in the futures-0.3 version of AsyncRead -- is that a conscious decision?

It seems to me that EOF is a real thing for many kinds of readers (both files and also network streams like TCP or QUIC streams). I was thinking that it might make sense to define the return type as Poll<Result<Option<usize>>> instead, so that there is the analogy with a Stream and a clearly defined marker for end of stream.

@Nemo157
Copy link
Member

Nemo157 commented May 20, 2019

AsyncRead::poll_read should have the same bullet point added to it, that is how the current design works. (Although, that is subtly wrong, as mentioned in the std::io::Read::read docs "[or] the buffer specified was 0 bytes in length.").

If you're going to go that far for type safety would Poll<Result<Option<NonZeroUsize>>> be better (although I guess this runs up against zero-sized buffer issues)?

The current design is consistent with std::io::Read, is it more useful to provide a more type safe design like this, or keep consistency with the closest equivalent std API which users are likely to be familiar with?

@djc
Copy link
Author

djc commented May 20, 2019

I understand wanting to align with the std::io::Read design. In that case, it would probably be helpful to clarify the use of the sentinel value in the documentation.

Marwes pushed a commit to Marwes/futures-rs that referenced this issue Jun 27, 2019
Fixes the inconsistency between `future::empty` and `stream::empty`.
I went with `pending` over `never` since there are already a future
called `Never` in the library.

Closes rust-lang#1624
Marwes pushed a commit to Marwes/futures-rs that referenced this issue Jun 27, 2019
Fixes the inconsistency between `future::empty` and `stream::empty`.
I went with `pending` over `never` since there are already a future
called `Never` in the library.

Closes rust-lang#1624
Marwes pushed a commit to Marwes/futures-rs that referenced this issue Jun 27, 2019
Fixes the inconsistency between `future::empty` and `stream::empty`.
I went with `pending` over `never` since there are already a future
called `Never` in the library.

Closes rust-lang#1624
cramertj pushed a commit that referenced this issue Jun 27, 2019
Fixes the inconsistency between `future::empty` and `stream::empty`.
I went with `pending` over `never` since there are already a future
called `Never` in the library.

Closes #1624
@Ralith
Copy link
Contributor

Ralith commented Jun 27, 2019

Was closing this intentional? AsyncRead doesn't seem to have been involved in that commit at all.

@Nemo157 Nemo157 reopened this Jun 27, 2019
@Nemo157
Copy link
Member

Nemo157 commented Jun 27, 2019

Nope, that was meant to refer to #1684 instead.

@cramertj cramertj added the docs label Oct 31, 2019
@taiki-e taiki-e added the A-io Area: futures::io label Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-io Area: futures::io docs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants