Implement IHttpResponseCompletionFeature.CompleteAsync for Kestrel HTTP/1.x #11195
Labels
area-networking
Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
enhancement
This issue represents an ask for new feature or an enhancement to an existing one
feature-kestrel
Milestone
Comments
Tratcher
added
enhancement
|
My feeling is this won't make 3.0. |
|
Was this done with #12328? |
|
Not necessarily, Http1 and Http2 have fairly different code paths. Give it a try? What's your interest? |
|
gRPC-Web allows for gRPC over HTTP1/1. One of its features is deadlines, which will call CompleteAsync. I have a functional test over HTTP/1.1 and I'm getting an odd result. Sometimes the HTTP request doesn't return any data when CompleteAsync is called. The client then errors with "The response ended prematurely." |
|
Test logs (failure happens in the final request): |
|
Wireshark log. Error happens in the final request. |
|
There's no response for that final request. I don't see any indication from the logs as to why. |
|
I think I know what the issue is: endpoints.Map("CompleteAsyncError", async context =>
{
System.IO.Pipelines.ReadResult result;
do
{
result = await context.Request.BodyReader.ReadAsync();
context.Request.BodyReader.AdvanceTo(result.Buffer.End);
} while (result.IsCompleted);
_ = RunCompleteAsync(context);
await Task.Delay(500);
await context.Response.BodyWriter.WriteAsync(Encoding.UTF8.GetBytes("Hello world"));
});
private async Task RunCompleteAsync(HttpContext context)
{
await Task.Delay(10);
await context.Response.BodyWriter.FlushAsync();
await context.Response.BodyWriter.WriteAsync(Encoding.UTF8.GetBytes("Hello world"));
await context.Response.CompleteAsync();
context.Abort();
}The issue shows up when |
|
Ah. Abort in HTTP/1 closes the whole connection so that makes sense. That said, you're doing unsafe multi-threading in this sample... |
|
I was reproducing gRPC deadline conditions which are pretty unsafe. My guess was they were the problem. However it still happens when multi-threading is removed: endpoints.Map("CompleteAsyncError", async context =>
{
System.IO.Pipelines.ReadResult result;
do
{
result = await context.Request.BodyReader.ReadAsync();
context.Request.BodyReader.AdvanceTo(result.Buffer.End);
} while (result.IsCompleted);
await Task.Delay(10);
await context.Response.BodyWriter.FlushAsync();
await context.Response.BodyWriter.WriteAsync(Encoding.UTF8.GetBytes("Hello world"));
await context.Response.CompleteAsync();
context.Abort();
}); |
|
I worked around this issue by adding a fake
|
Yeah, mocking out Reset to avoid an Abort is probably good for your scenario. |
|
Clearing the milestone. It looks like we have some 5.x work to do for this scenario. |
|
This should already work. HttpResponsePipeWriter.ComplateAsync calls HttpProtocol.CompleteAsync which in turn call HttpProtocol.ProduceEnd which flushes the chunked terminator if necessary. It wasn't until I recently merged #22575 to master that HttpResponsePipeWriter.CompleteAsync returned a Task that representing the completion of ProduceEnd, but flushing the connection PipeWriter doesn't guarantee that the chunked terminator has been written to the socket or acknowledged by the client anyway. Even after the change, aborting the HttpContext right after CompleteAsync could result in the client not receiving a chunked terminator. Content-Length checks should happen though. |
|
Looks like I fixed this way back in 3.0.0-preview7 a couple weeks after the issue was filed and missed closing this issue. I even added a WhenAppWritesLessThanContentLengthCompleteThrowsAndErrorLogged test (#11675). I just opened #26161 to add a ResponseBodyWriterCompleteFlushesChunkTerminator test. |
ghost
locked as resolved and limited conversation to collaborators
amcasey
added
area-networking
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
#11193 added IHttpResponseCompletionFeature.CompleteAsync for HTTP/2 gRPC scenarios, allowing the response to be completed without waiting for the request delegate to unwind.
There have been multiple asks for the same behavior for HTTP/1.1 chunked responses. Content-Length checks could also be done.
The text was updated successfully, but these errors were encountered: