Provide better experience when the BodyReader is left in a reading state

[halter73]

When middleware calls HttpContext.Request.BodyReader.ReadAsync() and exits without calling HttpContext.Request.BodyReader.AdvanceTo(), Kestrel will fail to read the following request while logging the following exception as a warning (at least for Content-Length HTTP/1.1 requests, not sure about others):

System.InvalidOperationException
  HResult=0x80131509
  Message=Reading is already in progress.
  Source=Microsoft.AspNetCore.Server.Kestrel.Core
  StackTrace:
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.Http1ContentLengthMessageBody.TryReadInternal(ReadResult& readResult) in /_/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs:line 126

This is pretty common type of error when something throws in between calls to ReadAsync() and AdvanceTo() and AdvanceTo() isn't in a finally block.

It would be better if Kestrel logged a warning that explicitly tells the developer that there's a missing call to BodyReader.AdvanceTo(). Better yet would be to recover from this situation by draining the rest of the request despite the BodyReader being left in a bad state so that the connection can still be used for future requests.

We should also ensure Kestrel handles the BodyWriter being left in a bad state gracefully.

See #17723 and #14727 (In that case it was Kestrel itself throwing without properly advancing the connection pipe though).

• ASP.NET Core version 3.1

[strawberries73]

Hello, I am new to Open Source. I would like to work on this issue.

[jkotalik]

@strawberries73 apologies, didn't see this notification. @halter73 I believe this issue is specifically about changing the error message for when we throw an exception during draining, right? I'd imagine an exception would be thrown here: https://github.com/dotnet/aspnetcore/blob/master/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs#L140, which would then be caught here

aspnetcore/src/Servers/Kestrel/Core/src/Internal/Http/Http1MessageBody.cs

Line 55 in bda52f7

AdvanceTo(readResult.Buffer.End);

. Changing that error message is what we'd need to do?

[halter73]

I think we should be tracking whether the BodyReader/BodyWriter was left in a reading/writing state without having to rely on catching an InvalidOperationException. It's confusing to log an "application" exception that the application had no opportunity to catch. I'm not quite sure what the log message should be. Maybe something like the following:

BodyReader.ReadAsync() was called without a corresponding call to BodyReader.AdvanceTo(). This can often be fixed using try/finally.