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

RecvStream::is_end_stream() shouldn't return true after encountering error #806

Open
eaufavor opened this issue Nov 1, 2024 · 2 comments · May be fixed by #810
Open

RecvStream::is_end_stream() shouldn't return true after encountering error #806

eaufavor opened this issue Nov 1, 2024 · 2 comments · May be fixed by #810
Labels

Comments

@eaufavor
Copy link
Contributor

eaufavor commented Nov 1, 2024

From the doc

Returns true if the receive half has reached the end of stream.
A return value of true means that calls to poll and poll_trailers will both return None.

However in reality, it would return true also after the connection / stream encounters errors because

if !stream.state.is_recv_closed() {
return false;
}

and is_recv_closed is true regardless of the actual cause of the closure
pub fn is_recv_closed(&self) -> bool {
matches!(
self.inner,
Closed(..) | HalfClosedRemote(..) | ReservedLocal
)

The problem of RecvStream::is_end_stream() returning true after encountering error is that it hints the user that the http request finishes successfully. So incomplete responses might be interpreted as complete ones.

My suggestion is that this function should only return true after seeing END_STREAM flag.

@seanmonstar
Copy link
Member

The problem of RecvStream::is_end_stream() returning true after encountering error is that it hints the user that the http request finishes successfully. So incomplete responses might be interpreted as complete ones.

I think you're right. Want to submit a pull request fixing this?

@eaufavor
Copy link
Contributor Author

eaufavor commented Nov 4, 2024

Yes will do. Thanks for the confirmation.

eaufavor added a commit to eaufavor/h2 that referenced this issue Nov 4, 2024
…received

Before this change, it returned true on other types of disconnection as
well.

Fixes hyperium#806
eaufavor added a commit to eaufavor/h2 that referenced this issue Nov 4, 2024
…received

Before this change, it returned true on other types of disconnection as
well.

Fixes hyperium#806
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants