Aspnetcore: Override ReadToEndAsync in HttpRequestStreamReader

[davidfowl]

https://github.com/aspnet/AspNetCore/issues/4707#issuecomment-529565855

When doing using HttpRequestStreamReader.ReadToEndAsync it's possible to end up doing blocking reads since we don't override ReadAsync(Memory<char>)

[anurse]

We actually need to directly override ReadToEndAsync since it just Task.Runs ReadToEnd(): https://source.dot.net/#q=TextReader.ReadToEndAsync

[rgueldenpfennig]

I'm a bit surprised that this bug is planned to be fixed in the 5.0.0 milestone. Due to #7644 we changed our code from

using (var reader = new HttpRequestStreamReader(context.Request.Body, encoding: Encoding.UTF8))
var body = await reader.ReadToEndAsync();

to

using (var reader = new StreamReader(context.Request.Body, Encoding.UTF8, leaveOpen: true))
var body = await reader.ReadToEndAsync();

To prevent IO from blocking.

Is this a recommended workaround?

[thomaslevesque]

That's really bad... Definitely a bug. Any chance it could be fixed earlier than 5.0.0? I'm willing to submit a PR for this.

[anurse]

Apologies for the delay in response, this fell off my radar a bit and then the holidays hit :).

Is this a recommended workaround?

@rgueldenpfennig Yes, that would be the recommended workaround.

Any chance it could be fixed earlier than 5.0.0? I'm willing to submit a PR for this.

@thomaslevesque In general, we are very conservative about what we fix in patch releases (3.1.x). That doesn't mean we wouldn't do it, but it's a decision that's made at a higher level (coordinated with all the other parts of .NET Core) ;). If you're still interested in sending a PR, I'd suggest submitting one to make the fix in master. We can open a new issue to track a request to backport the change to the next 3.1.x patch and submit that for servicing approval.

[thomaslevesque]

@anurse done: #18232

[halter73]

We actually need to directly override ReadToEndAsync since it just Task.Runs ReadToEnd():

It looks like TextReader calls ReadAsyncInternal() in a loop. Problem is that ReadAsyncInternal() is implemented by Task.Factory.StartNewing Read(). So effectively the same thing.

The problem is it also looks like we need to override the following, most of which also call into ReadAsyncInternal(), in order to avoid async-over-sync:

• public virtual Task<string?> ReadLineAsync()
• public virtual ValueTask<int> ReadAsync(Memory<char> buffer, CancellationToken cancellationToken = default)
  
  
  
  • This is still async if MemoryMarshal.TryGetArray(buffer, ... succeeds, but sync otherwise
  
  
• public virtual Task<int> ReadBlockAsync(char[] buffer, int index, int count)
• public virtual ValueTask<int> ReadBlockAsync(Memory<char> buffer, CancellationToken cancellationToken = default)

[halter73]

@davidfowl @anurse Why aren't we trying to make these improvements to TextReader itself? Do we know for some reason this kind of change wouldn't be accepted in the runtime repo?

[anurse]

@halter73 I don't know that they wouldn't be accepted. We could open an issue and ask.

[anurse]

Pre-planning: Runtime issue is in Future. If someone is going to fix this in 5.0 it'll have to be us. Given our resource constraints this might be something to defer to a later preview and see if there's interest in a community PR.

[davidfowl]

@jkotalik isn't this done?

[BrennanConroy]

@jkotalik says this should be done!