fix(client): reading trailers shouldn't propagate NO_ERROR from early response#3998
fix(client): reading trailers shouldn't propagate NO_ERROR from early response#3998ulyssa wants to merge 2 commits intohyperium:masterfrom
NO_ERROR from early response#3998Conversation
ulyssa
force-pushed
the
early-response-trailers-1.x
branch
from
7afc220 to
f25a96a
Compare
src/body/incoming.rs
Outdated
| Err(e) => { | ||
| match e.reason() { | ||
| // These reasons should cause reading the trailers to stop, but not fail it. | ||
| // The same logic as for `Read for H2Upgraded` is applied here. |
There was a problem hiding this comment.
I realize this is just bring down the logic that already exists for data frames, and the code itself looks fine. And the unit tests make sense why they want to suppress the error.
But, reading it all again, I have a worry: couldn't this also be swallowing real cancellations, and the user thinks they got a full body, when they got a truncated one?
There was a problem hiding this comment.
Looking over RFC 7540, I believe you're right that both this and the above branch should only be looking at Reason::NO_ERROR, and shouldn't be dropping the Reason::CANCEL. The section on early responses in 8.1 only mentions NO_ERROR:
An HTTP response is complete after the server sends -- or the client
receives -- a frame with theEND_STREAMflag set (including any
CONTINUATIONframes needed to complete a header block). A server can
send a complete response prior to the client sending an entire
request if the response does not depend on any portion of the request
that has not been sent and received. When this is true, a server MAY
request that the client abort transmission of a request without error
by sending aRST_STREAMwith an error code ofNO_ERRORafter sending
a complete response (i.e., a frame with theEND_STREAMflag).
Clients MUST NOT discard responses as a result of receiving such a
RST_STREAM, though clients can always discard responses at their
discretion for other reasons.
The only time that sending CANCEL is mentioned is under the description of push responses in 8.2.2:
If the client determines, for any reason, that it does not wish to
receive the pushed response from the server or if the server takes
too long to begin sending the promised response, the client can send
a RST_STREAM frame, using either the CANCEL or REFUSED_STREAM code
and referencing the pushed stream's identifier.
This section may be why the code was added to the match in #3275, but I agree that it does sound like it should be propagated so that the server is aware of the cancellation, as opposed to NO_ERROR where the RFC says that clients must not discard the response. I'll update this and #3999 to not filter out the Reason::CANCEL error.
2 participants
This is the trailers variant of the fix for reading the body in #3275, so that it is possible to both attempt to read the body and the trailers when the server has sent a
RST_STREAMwithNO_ERRORafter its response to indicate that the client should stop attempting to send it the body.I have added a trailers-only variant of
http2_responds_before_consuming_request_bodythat fails without the fix, and also updatedhttp2_responds_before_consuming_request_bodyto verify that it can check whether there are any trailers.