dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.35k stars 9.99k forks source link

Kestrel always reads request body even if not read by user code #24732

Closed cmeeren closed 4 years ago

cmeeren commented 4 years ago

It seems to me that Kestrel reads the request body even if no user code reads it. This causes difficulties for me since I'm trying to implement a large file upload endpoint that streams directly to disk, and which should skip the upload if e.g. authentication fails (which is checked before reading the response body).

Please see this very simple ASP.NET Core 3.1 repro solution (it's using Giraffe because that's what I know, but it's only used in a trivial way, simply returning the text "test" for all requests):

Test.zip

For reference, here's the code:

module Test.App

open Microsoft.AspNetCore.Hosting
open Microsoft.Extensions.Hosting
open Microsoft.Extensions.Logging
open Giraffe

[<EntryPoint>]
let main args =
  Host.CreateDefaultBuilder(args)
      .ConfigureWebHostDefaults(fun builder ->
        builder
          .Configure(fun app -> app.UseGiraffe(text "test"))
          .ConfigureServices(fun services -> services.AddGiraffe() |> ignore)
          .ConfigureLogging(fun builder ->
            builder.SetMinimumLevel(LogLevel.Trace).AddConsole().AddDebug() |> ignore)
        |> ignore)
      .Build()
      .Run()
  0

Repro:

  1. Run the test project
  2. Use Postman or similar to send any request containing a body (even a GET request will reproduce this). For example: image

When performing the request, the following log entries are printed. Note that after the Giraffe middleware completes and ASP.NET Core returns a 200 response, there are two entries at the very end from Microsoft.AspNetCore.Server.Kestrel that indicate Kestrel has read the request body.

info: Microsoft.AspNetCore.Hosting.Diagnostics[1]
      Request starting HTTP/1.1 GET http://localhost:5000/ text/plain 3
dbug: Microsoft.AspNetCore.HostFiltering.HostFilteringMiddleware[0]
      Wildcard detected, all requests with hosts will be allowed.
trce: Microsoft.AspNetCore.HostFiltering.HostFilteringMiddleware[2]
      All hosts are allowed.
dbug: Giraffe.Middleware.GiraffeMiddleware[0]
      Giraffe returned Some for HTTP/1.1 GET at / in 8.178
dbug: Microsoft.AspNetCore.Server.Kestrel[9]
      Connection id "0HM1SVP19RFEC" completed keep alive response.
info: Microsoft.AspNetCore.Hosting.Diagnostics[2]
      Request finished in 20.8991ms 200 text/plain; charset=utf-8
dbug: Microsoft.AspNetCore.Server.Kestrel[25]
      Connection id "0HM1SVP19RFEC", Request id "0HM1SVP19RFEC:00000001": started reading request body.
dbug: Microsoft.AspNetCore.Server.Kestrel[26]
      Connection id "0HM1SVP19RFEC", Request id "0HM1SVP19RFEC:00000001": done reading request body.
Tratcher commented 4 years ago

This is required by the http protocol. The body may already be on the wire so I the server needs to drain it in order to get to the next request. The alternative is to close the connection. Note kestrel will only attempt to drain for 5s before closing the connection.

A few options for this scenario: A) enable 100-continue on the client, this can give you a chance to authenticate before sending the body. This only works for some kinds of auth. B) add a Connection: close header to the response when you don't want the server to drain it. C) use http/2 where the server can reset an individual request steam without closing the connection.

cmeeren commented 4 years ago

Well, you learn something new every day :)

B) add a Connection: close header to the response when you don't want the server to drain it.

Can this have any important side effects I should know of? Can I safely just include this in all the file upload requests, to improve performance by not draining the stream whenever file upload fails for any reason (early auth, file system error, etc.)?

C) use http/2 where the server can reset an individual request steam without closing the connection.

If the server supports HTTP/2 and the client uses HTTP/2, are you saying the issue will be automatically fixed without needing any other change? If so, referring to B), should I add Connection: close only if the client does not use HTTP/2?

Tratcher commented 4 years ago

Well, you learn something new every day :)

B) add a Connection: close header to the response when you don't want the server to drain it.

Can this have any important side effects I should know of? Can I safely just include this in all the file upload requests, to improve performance by not draining the stream whenever file upload fails for any reason (early auth, file system error, etc.)?

Closing and opening new connections has significant performance impacts, but less so than uploading a large file. That's why the server automatically closes the connection if it can't drain in 5s. Closing it yourself is optional.

C) use http/2 where the server can reset an individual request steam without closing the connection.

If the server supports HTTP/2 and the client uses HTTP/2, are you saying the issue will be automatically fixed without needing any other change? If so, referring to B), should I add Connection: close only if the client does not use HTTP/2?

Correct.

cmeeren commented 4 years ago

While B) would be my preferred option (the only option I can control, really), I can find no way of actually forcing it into the response. It is sent when debugging locally, but when running on IIS or even in Azure App Service on Linux, the header is not present in the response. I have googled but not found any way to fix this. Do you happen to know if it's even possible to force this header through IIS or Azure App Service on Linux?

ghost commented 4 years ago

This issue has been resolved and has not had any activity for 1 day. It will be closed for housekeeping purposes.

See our Issue Management Policies for more information.