Mid-DATA stream reset #2278
Mid-DATA stream reset #2278
Conversation
|
@icing can you try this branch out to see if it addresses your failing curl tests? |
|
Thanks @LPardue , with this patch I can no longer reproduce the bug. I'll disable the test in curl until we see a quiche version higher than 0.24.4. |
Let nhttpx only use http/1.1 to backend. This reproduces the bug in quiche with higher frequency. Allow test_14_05 to now return a 400 in addition to the 431 we get from a h2 backend to nghttpx. Skip test_05_02 in h3 on quiche not newer than version 0.24.4 in which its bug is fixed (cloudflare/quiche#2278)
Let nghttpx only use http/1.1 to backend. This reproduces the bug in quiche with higher frequency. Allow test_14_05 to now return a 400 in addition to the 431 we get from a h2 backend to nghttpx. Skip test_05_02 in h3 on quiche not newer than version 0.24.4 in which its bug is fixed: cloudflare/quiche#2278 Ref: cloudflare/quiche#2277 Closes #19770 (original Issue) Closes #19916
LPardue
force-pushed
the
lucas/mid-data-reset-as-finished
branch
from
8f79de7 to
0167337
Compare
| // immediate Event::Finished if an application has already | ||
| // read exactly the stream final size. | ||
| if let Err(crate::Error::StreamReset(e)) = | ||
| conn.stream_recv(stream_id, &mut []) |
There was a problem hiding this comment.
Are there other ways to query if the stream was reset?
I think that this conn.stream_recv will trigger a call to self.streams.remove_readable and collect the stream if it has completed.
Does something elsewhere in the code depend on the stream being listed there in order to properly handle the stream reset?
There was a problem hiding this comment.
This is similar to how we detect if the stream was reset here
There's no other way to check that without, I beleive, without changing the quiche library to track and expose the state, which probably raises evem more questions.
I think it's important for the quiche state to get cleaned up, to prevent duplicate or erroneous events being raised (this issue is one case where that's happened but there's been others in the past, leading to the already convoluted code)
There was a problem hiding this comment.
This may be fine, although I'm curious what application hooks are or should be invoked when a stream is reset or finishes gracefully. Won't block the PR on this.
There was a problem hiding this comment.
Well, that's up to the app to deal with when consuming the result of poll() 😄
Some scenarios:
- Client resets a POST early on its own according.
- Client resets GET because server rejected it with STOP_SENDING
- Server resets a response because the origin abruptly terminated the response for whatever reason (the scenario identified in Stefan's issue)
It's important to distinguish between reset or finished especially with respect to if there is a content-length declared or not. We delegate a lot of the semantic responsibility to apps. In turn we should be expected to pass on the truth of what's happened "on the wire".
3 participants
This attempts to use a unit test to emulate the behaviour described in #2277, where the peer clams to send a DATA frame of length N but sends fewer bytes than that and resets the stream. This seems to trigger an erroneous Event::Finished event that confuses applications.
Ideally, we fix the underlying logic and thus will need to change the test to match.