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

fix(client): early server response shouldn't propagate NO_ERROR #3275

Merged

Conversation

DDtKey
Copy link
Contributor

@DDtKey DDtKey commented Jul 23, 2023

Closes #2872

Added test is failing against the current version (before this fix)

Backport for 0.14.x: #3274

Closes hyperium#2872

Added test is failing against version without these changes.
Some(Err(e)) => {
return match e.reason() {
// These reasons should cause stop of body reading, but don't fail it.
// The same logic as for `Read for H2Upgraded` is applied here.
Copy link
Contributor Author

@DDtKey DDtKey Jul 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm referring to this line here:

Some(Reason::NO_ERROR) | Some(Reason::CANCEL) => Ok(()),

And it makes sense in terms of protocol behavior for the reasons.

Both of them call the same operation (self.recv_stream.poll_data(cx)) and seems that logic should be the same. But we can make it more specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, an additional example from another lang & lib where it was also solved by avoiding error if response has been read: square/okhttp#6295

@DDtKey DDtKey changed the title fix: early respond from server shouldn't propagate reset error fix: early server response shouldn't propagate reset error Jul 23, 2023
@DDtKey DDtKey changed the title fix: early server response shouldn't propagate reset error fix: early server response shouldn't propagate NO_ERROR Jul 23, 2023
@DDtKey DDtKey changed the title fix: early server response shouldn't propagate NO_ERROR fix(client): early server response shouldn't propagate NO_ERROR Jul 23, 2023
@seanmonstar seanmonstar requested a review from nox July 24, 2023 18:46
Copy link
Contributor

@nox nox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to me given we already do something similar for streams upgraded from HTTP/2.

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

Successfully merging this pull request may close these issues.

Client: handle RST_STREAM with NO_ERROR set for the reason
3 participants