-
Notifications
You must be signed in to change notification settings - Fork 9.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Override ReadToEndAsync in HttpRequestStreamReader #13834
Comments
We actually need to directly override |
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
to
To prevent IO from blocking. Is this a recommended workaround? |
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. |
Apologies for the delay in response, this fell off my radar a bit and then the holidays hit :).
@rgueldenpfennig Yes, that would be the recommended workaround.
@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 |
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:
|
@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? |
@halter73 I don't know that they wouldn't be accepted. We could open an issue and ask. |
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. |
@jkotalik isn't this done? |
@jkotalik says this should be done! |
#4707 (comment)
When doing using
HttpRequestStreamReader.ReadToEndAsync
it's possible to end up doing blocking reads since we don't overrideReadAsync(Memory<char>)
The text was updated successfully, but these errors were encountered: