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.

Sign up for GitHub

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

Jump to bottom

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
easy

Comments

@eaufavor
Copy link
Contributor

eaufavor commented Nov 1, 2024 •
edited
Loading

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

h2/src/proto/streams/recv.rs

Lines 560 to 562 in 77be664

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

and is_recv_closed is true regardless of the actual cause of the closure

h2/src/proto/streams/state.rs

Lines 416 to 420 in 77be664

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
@eaufavor
Fix RecvStream::is_end_stream(): return true only when END_STREAM is …
0327d9f 
…received

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

Fixes hyperium#806
@eaufavor eaufavor linked a pull request Nov 4, 2024 that will close this issue
Open
eaufavor added a commit to eaufavor/h2 that referenced this issue Nov 4, 2024
@eaufavor
Fix RecvStream::is_end_stream(): return true only when END_STREAM is …
dad7572 
…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 12, 2024
@eaufavor
Fix RecvStream::is_end_stream(): return true only when END_STREAM is …
1857605 
…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
Assignees
No one assigned
Labels
easy
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix RecvStream::is_end_stream(): return true only when END_STREAM is received eaufavor/h2
2 participants
@seanmonstar @eaufavor