Thread starvation due to JsonInputFormatter reading request stream synchronously

[azhmur]

Describe the bug

JsonInputFormater use JsonSerializer to read request stream synchronously, thus consuming many threads if many slow requests are coming in the same time. If threadpool was relatively small before that it may not expand in time resulting all thread pool threads are blocked.

JsonInputFormater has workaround with calling EnableBuffering and DrainAsync before passing stream to synchronous part of JsonSerilizer, but in case someone already called EnableBuffering or EnableRewind before that (in our case this is done for request tracing in custom middleware), this workaround doesn't work.

Synchrounous stream read because of calling synchronous JsonFormatter.Deserialize from async method:
https://github.com/aspnet/AspNetCore/blob/master/src/Mvc/src/Microsoft.AspNetCore.Mvc.NewtonsoftJson/NewtonsoftJsonInputFormatter.cs#L210

Workaround which fails because Request Stream already seekable (but not drained asynchronously!):
https://github.com/aspnet/AspNetCore/blob/master/src/Mvc/src/Microsoft.AspNetCore.Mvc.NewtonsoftJson/NewtonsoftJsonInputFormatter.cs#L132

As far as I understand NewtonsoftJson is going to deliver Pipe based asynchronous version in future which will remove nascesseity for this workaround.

My supposed fix for this workaround (can provide PR):

        if (!request.Body.CanSeek && !suppressInputFormatterBuffering)
        {
            // JSON.Net does synchronous reads. In order to avoid blocking on the stream, we asynchronously
            // read everything into a buffer, and then seek back to the beginning.
            request.EnableBuffering();
            Debug.Assert(request.Body.CanSeek);
        }

        if (!suppressInputFormatterBuffering)
        {
            await request.Body.DrainAsync(inputFormatterContext.HttpContext.RequestAborted);
            request.Body.Seek(0L, SeekOrigin.Begin);
        }

[pranavkm]

Thanks for your report. We're tracking the work here via #6397.

[pranavkm]

@azhmur the bar to patch 2.1 or 2.2 is pretty high and it's very unlikely we would accept the PR you sent in particular if if there's a reasonable workaround you could use.
In your particular case, are you able to write a middleware that drains the request prior to invoking MVC? Would that would effectively solve your issue?

[azhmur]

Yes, we have fixed issue within our code. I had created PR, because investigation have taken serious efforts (like all thread starvation bugs do), so I want to help others not to spend their time on debugging similar issues.

[mkArtakMSFT]

Closing per @pranavkm's response.