fix(client): early server response shouldn't propagate NO_ERROR by DDtKey · Pull Request #3275 · hyperium/hyper
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. |
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.
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
changed the title
fix: early respond from server shouldn't propagate reset error
fix: early server response shouldn't propagate reset error
DDtKey
changed the title
fix: early server response shouldn't propagate reset error
fix: early server response shouldn't propagate NO_ERROR
DDtKey
changed the title
fix: early server response shouldn't propagate NO_ERROR
fix(client): early server response shouldn't propagate NO_ERROR
nox approved these changes Jul 26, 2023
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters