QUIC: Read data and get end notification together #55707
Comments
dotnet-issue-labeler
bot
added
area-System.IO
untriaged
|
Tagging subscribers to this area: @dotnet/area-system-io Issue DetailsI don't know whether this is important, but there doesn't seem to be an API on An API for writing data and ending a stream together exists: await stream.WriteAsync(data, endStream: true);But for reading you must always get data and then read again to get a result of 0 which means no more data: var buffer = GetBuffer();
while (true)
{
var readCount = await stream.ReadAsync(buffer);
if (readCount == 0) break;
// Do stuff with data but don't know whether it is the final data.
}
|
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsThere doesn't seem to be an API on An API for writing data and ending a stream together exists: await stream.WriteAsync(data, endStream: true);But for reading you must always get data and then read again to get a result of 0 which means no more data: var buffer = GetBuffer();
while (true)
{
var readCount = await stream.ReadAsync(buffer);
if (readCount == 0) break;
// Do stuff with data but don't know whether it is the final data.
}I think this might be required for Kestrel to properly support HEADERS only requests. When Kestrel gets the first HEADER frame it needs to start the request. But it also needs to know whether it or not is it ONLY a HEADERS request. If the stream data ends with the HEADERS frame then we process it differently than if the stream is still going and there are DATA frames still to come. Kestrel can't hang around, waiting on the next
|
|
We could potentially expose an API like this, but I don't think it solves the problem here. You will still need to handle the case where the headers arrive without a FIN, and then the FIN arrives later. |
|
Related discussion: #55347 (comment). MSQuic looks like it surfaces this state on StreamEventDataReceive.Flags = FIN, QuicStream does not. |
|
My knowledge of QUIC isn't strong, but I believe that in QUIC the data and the end stream flag will always arrive together, like how a HEADERS frame in HTTP/2 would have an END_STREAM flag set or not. STREAM frame? - https://www.rfc-editor.org/rfc/rfc9000.html#section-19.8 What we need with QuicStream is to know whether the data that has come in has had the FIN set. Right now we don't have that capability. Now, if someone...
...then that is a problem with the peer. Kestrel will report an error when it validates the incoming request. |
That's not the case. In QUIC, it is perfectly legal to send some data, wait, then send FIN.
QUIC packets/frames aren't really analogous to HTTP2 frames. They are much more similar to TCP packets. HTTP3 frames are analogous to HTTP2 frames -- HEADERS, DATA, etc.
Unless there is some particular provision of HTTP3 that outlaws this that I'm not aware of, then this is a perfectly fine thing to do and Kestrel needs to handle it. Note this is somewhat analogous to sending an HTTP2 HEADERS frame without EndStream, then immediately sending a 0-length DATA frame with the FIN bit set. |
BTW, I don't think there is a notion of a "HEADERS only request" in HTTP3, or really in HTTP itself. There are only requests with 0-length bodies. There are some ways you can detect a 0-length body just from inspecting the headers -- for example, if the Content-Length header is set to 0. But that's not always the case, regardless of version. |
It is part of the HTTP/3 spec but it is not spelled out obviously. We need it to properly detect and reject certain malformed requests. An example is someone sending a non-zero content-length in a message with no body. We need to reject with an abort, and we need to do it before executing user app code. https://datatracker.ietf.org/doc/html/draft-ietf-quic-http-34#section-4.1.3 |
I'm not seeing it here, can you point to what you mean specifically? |
Scenario: we have HEADERS, they have a content-length of 10, we know we have no DATA frames (the stream has ended), therefore the request is malformed at the point in time the HEADERS frame is received. App code is never executed with the malformed request.
Kestrel is acting as a proxy and should not forward on the malformed request. We stop that from happening by rejecting the request when the HEADERS are received and never executing app code (i.e. the proxy code that will forward the request) |
|
Sure, that makes sense. But how does that apply to the issue at hand here? How is it different than, say, the request body having 1 byte instead of 0? |
|
Kestrel will start executing the app code and I believe will error at a different place (on reading the end of the request body? Chris will know). There is no way for a server to pre-emptively detect the request is malformed in that situation. In the case of the no message body a server can detect it is malformed immediately and reject it before executing app code. |
|
Ok, but again, I think you have to handle the case where FIN is sent in a separate packet. I've yet to see anything that says otherwise. Am I missing something?
That sounds nice, but... there are lots of ways a request can be malformed. Why is this case special? |
|
I also want to point out that, while there are scenarios where we know there is no request body a priori, there are also scenarios where we simply don't know. For example, we ask the user's content to serialize, and it results in 0 bytes. In those cases it's possible and in fact likely that you will receive the FIN separate from the request headers, and it is not correct to reject this as malformed. |
In that case Kestrel will assume there is a body to go with the request header's content-length and will execute the user app code. |
|
Okay, but what is your alternative here? If this is valid client behavior (and I believe it is), then you need to handle it somehow. |
|
I don't understand the question. Do you mean this?
I'm guessing the client didn't send a content-length so it doesn't matter whether there is a body present or not. We have many tests for different content-length scenarios to test we're handling them correctly - https://github.com/dotnet/aspnetcore/blob/main/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs |
|
You claimed above:
Based on my understanding, this is valid client behavior and you need to handle it. |
|
It will be just like an empty DATA frame with END_STREAM in HTTP/2. The request will make it to user app code and the error will happen later. |
|
Okay, then it seems like we have this handled. |
|
Triage: seems like an API and feature work. Unless we have a failing scenario from a user/customer (H/3 client server) we should leave it to 7.0. |
ManickaP
added
the
api-suggestion
|
I believe I've narrowed down some flakey tests to missing this API. There are Kestrel tests that are sent GET requests by the client (and so have no request body). The app code does some minimal work on the server. The tests pass is the request was completed gracefully. That means the request was read to the end and the response completes sending Unfortunately, the app code is completing BEFORE the end of the request. That's because we're getting the content to read the HEADERS frame in one There is a race between the app code completing and then second
That happens despite there being no request body to read. |
|
Regardless of whether we add this API, it's still valid HTTP3 to send the FIN separately and you will need to handle this case somehow. |
|
That's fine. Kestrel won't break or error, but we will be where we are today for all GET requests: there will be a race. Apps will write logs that GET requests weren't completed cleanly which will be confusing. We can't fix it for sub-optimal clients, but we can stop this from happening from those that send HEADERS and FIN together. At the very least, that will include HttpClient - #55347 - and will fix the flakey HttpClient+Kestrel tests that we have. |
Why not? |
|
Because Kestrel starts processing the app delegate on a different thread as soon as HEADERS are received. The server can't hang around, waiting for the next After that point it is a race between IO thread figuring out there is no body when the next If you want to keep discussing this then I suggest a meeting. Explaining Kestrel's architecture and challenges one comment at a time will take forever. There is also a WIP branch that adds a QuicStream property that indicates whether you have read to the end of the stream. That will let us fix this. We could use some help with it. |
Why not? The server is just generating an event here, right? Why is it hard to defer generating this event until you determine whether there is actually a request body or not?
I'm not sure what this means; can you explain?
Great, let's set up a meeting. I will say, it seems strange to me that we would generate an event like this in a case where (a) the client is behaving correctly, as per RFC, and (b) the user's code on the server is also behaving reasonably. As a user, how is this event helpful to me? What sort of action should I take? |
ghost
added
the
in-pr
ghost
removed
the
in-pr
Add ReadsCompleted API that exposes ReadState=ReadsCompleted, set ReadState to ReadsCompleted if FIN flag arrives in RECEIVE event, fix ReadState changing to final stauses, expand ReadState transition description Fixes #55707
ghost
locked as resolved and limited conversation to collaborators
There doesn't seem to be an API on
QuicStreamthat allows you to read data and get a notification that it is the final chunk of data in one operation.An API for writing data and ending a stream together exists:
But for reading you must always get data and then read again to get a result of 0 which means no more data:
I think this might be required for Kestrel to properly support HEADERS only requests.
When Kestrel gets the first HEADER frame it needs to start the request. But it also needs to know whether it or not is it ONLY a HEADERS request. If the stream data ends with the HEADERS frame then we process it differently than if the stream is still going and there are DATA frames still to come.
Kestrel can't hang around, waiting on the next
QuicStream.ReadAsyncto return 0 or a value before it starts the request. That data might be a long time coming (e.g. a long-running gRPC stream where the client doesn't send a message on the stream until 1 minute later)I think we need
ReadAsyncto return something like ReadResult.ReadResulthas data on it (it could be a data length if you prefer), AND a flag saying whether we are done or not.The text was updated successfully, but these errors were encountered: