Skip to content
This repository has been archived by the owner on Nov 22, 2018. It is now read-only.

[Discussion]: synchronous request and response IO to be disallowed by default #86

Closed
halter73 opened this issue Jun 23, 2017 · 13 comments
Closed

Comments

@halter73
Copy link
Member

halter73 commented Jun 23, 2017

Discussion for aspnet/Announcements#252


Starting in ASP.NET Core 2.0.0 RTM, synchronous request and response IO will be disallowed by default.

For example, HttpContext.Request.Body.Read and HttpContext.Response.Body.Write will both throw InvalidOperationExceptions with a message communicating that either the equivalent async API should be called or synchronous IO should be explicitly allowed using IHttpBodyControlFeature.AllowSynchronousIO.

Synchronous IO hasn't been disallowed by default yet. After 2.0.0-preview2, there will be changes to both Kestrel and HttpSysServer to disallow this. Both servers will have a property added to their respective Options classes to globally allow synchronous IO.

Note: This change only impacts the request and response Stream APIs.

@Sebazzz
Copy link

Sebazzz commented Jun 24, 2017

What is the thinking behind this? Is it to encourage using async or does it actually have technical benefits?

Both servers will have a property added to their respective Options classes to globally allow synchronous IO.

@halter73 You probably mean only asynchronous IO.

@John0King
Copy link

Same question . Won't a new thread be started to excute our code when HttpContext be created ? what's the benefits of use async on an individual thread ?

@davidfowl
Copy link
Member

What is the thinking behind this? Is it to encourage using async or does it actually have technical benefits?

It is to discourage the use of synchronous IO by default. Kestrel's default transport libuv doesn't even have a way to write or read synchronously so it's very inefficient to call Read/Write (see https://blogs.msdn.microsoft.com/pfxteam/2012/04/13/should-i-expose-synchronous-wrappers-for-asynchronous-methods/). It can lead to scalability issues with the thread pool which negatively impacts your application.

Same question . Won't a new thread be started to excute our code when HttpContext be created ? what's the benefits of use async on an individual thread ?

User code runs on a thread pool thread by default if that's what you're asking. I'm not sure what you mean by "use async on an individual thread".

@John0King
Copy link

@davidfowl

User code runs on a thread pool thread by default

There already a thread , does that mean it already async ? This is what I'm confusing for .

@ThatRendle
Copy link

@John0King Async is not the same as parallel. Asynchronous operations allow a thread to be returned to the pool and reused while an I/O operation happens, often at a very low level in the OS. When the operation completes, your code resumes on another thread from the pool.

@John0King
Copy link

@markrendle Just curiosity
HttpContext exists in each request thread , How can synchronous operations block the IO of a Stream inside HttpContext ?

@benaadams
Copy link

@John0King Synchronous operations are a bit of a misnomer; the are really blocking operations, so they pause and hold onto the thread until completion; whereas async operations release the thread, then get assigned an available one from the pool on completion.

As the blocking operations hold onto the threads the number of threads in use (but doing nothing) keeps climbing, and will eventually start becoming rate limited, impacting throughput; whereas with async operations the number of threads generally stays fairly low and constant.

@John0King
Copy link

@benaadams @markrendle Thanks a lot

@MaximRouiller
Copy link

Will this method be branded obsolete and eventually phased out? Is that even a thing in .NET Core (ObsoleteAttribute)?

@John0King
Copy link

@MaximRouiller I don't think so , HttpContext.Response.Body and HttpContext.Request.Body is Stream abstraction class.

@sandersaares
Copy link

The announcement mentions the following:

incompatibilities this introduces with existing BCL types that are either difficult or impossible to work around without fully buffering.

What are these incompatibilities?

@halter73
Copy link
Member Author

halter73 commented Jun 30, 2017

The biggest issues we found so far now are StreamWriter and DeflateStream Dispose methods calling the non-async Flush and sometimes Write methods on their underlying Streams. Here's a ResponseCompression issue to investigate the DeflateStream issue further.

We've considered allowing non-async flushes, since explicit flush calls usually no-op anyway. We also know that for StreamWriter calling FlushAsync immediately before Dispose prevents the non-async Write call. But the initial problems we ran into with StreamWriter and DeflateStream has caused us to reconsider making such a breaking change this soon before RTM.

Before making this break, we'd like to go over our tests at least to see if there are any more common public APIs that make it difficult to avoid indirect synchronous stream API calls.

@muratg
Copy link
Contributor

muratg commented Aug 30, 2018

Discussion on this issue seems to have died. Closing this issue. If folks still have issues, please file another bug! Thanks.

@muratg muratg closed this as completed Aug 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants