Skip to content
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

Change KestrelServerOptions.AllowSynchronousIO default to false #4774

Closed
halter73 opened this issue Jun 29, 2017 · 14 comments
Closed

Change KestrelServerOptions.AllowSynchronousIO default to false #4774

halter73 opened this issue Jun 29, 2017 · 14 comments
Assignees
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions blocked The work on this issue is blocked due to some dependency breaking-change This issue / pr will introduce a breaking change, when resolved / merged. Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-kestrel

Comments

@halter73
Copy link
Member

KestrelServerOptions.AllowSynchronousIO is going to default to true for 2.0.0. Hopefully we can change this to false in our next major release.

aspnet/Announcements#252 (comment)

@muratg @Tratcher We'll probably want to file a similar issue for HttpSysServer.

@muratg
Copy link
Contributor

muratg commented Jun 29, 2017

@Tratcher could you file a bug on HttpSysServer to track this for 3.0.0?

@Tratcher
Copy link
Member

@davidfowl
Copy link
Member

We need to do this again in 3.0 https://github.com/dotnet/corefx/issues/32665

@aspnet-hello aspnet-hello transferred this issue from aspnet/KestrelHttpServer Dec 13, 2018
@aspnet-hello aspnet-hello added this to the 3.0.0 milestone Dec 13, 2018
@aspnet-hello aspnet-hello added area-servers enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-kestrel labels Dec 13, 2018
@davidfowl davidfowl modified the milestones: 3.0.0, 3.0.0-preview2 Dec 13, 2018
@davidfowl
Copy link
Member

@halter73 @muratg @Tratcher we should do this earlier rather than later.

@Tratcher
Copy link
Member

I just fixed response compression so this is unblocked.

@Tratcher Tratcher self-assigned this Dec 13, 2018
@Tratcher
Copy link
Member

So System.IO.StreamReader.EndOfStream calls sync Stream.Read...

>	Microsoft.AspNetCore.Server.IIS.dll!Microsoft.AspNetCore.Server.IIS.Core.HttpRequestStream.Read(byte[] buffer, int offset, int count) Line 30	C#
 	Microsoft.AspNetCore.Server.IIS.dll!Microsoft.AspNetCore.Server.IIS.Core.WrappingStream.Read(byte[] buffer, int offset, int count) Line 66	C#
 	System.Private.CoreLib.dll!System.IO.StreamReader.ReadBuffer() Line 618	C#
 	System.Private.CoreLib.dll!System.IO.StreamReader.EndOfStream.get() Line 304	C#
 	InProcessWebSite.dll!TestSite.Startup.ReadAndWriteEchoLines(Microsoft.AspNetCore.Http.HttpContext ctx) Line 351	C#

@halter73
Copy link
Member Author

Do we need a StreamReader.EndOfStreamAsync()? Or can we just avoid using this API and recommend other's do the same like with the other sync Stream(Reader) APIs?

@Tratcher
Copy link
Member

It looks easy enough to just keep reading and check for null.

@davidfowl
Copy link
Member

cc @stephentoub FYI

@Tratcher
Copy link
Member

@halter73 @davidfowl why didn't Kestrel block sync response Flush()? It can cause all of the same issues, no?

I tried blocking it but found that StreamWriter.Dispose always calls Flush.

@halter73
Copy link
Member Author

StreamWriter.Dispose() was the reason. aspnet/KestrelHttpServer#1919 (comment)

@Tratcher
Copy link
Member

All of the diagnostics pages do sync writes....

@muratg muratg modified the milestones: 3.0.0-preview2, 3.0.0-preview3 Jan 11, 2019
@muratg
Copy link
Contributor

muratg commented Jan 11, 2019

Required after this: #6397

@Tratcher Tratcher added the blocked The work on this issue is blocked due to some dependency label Jan 15, 2019
@Tratcher Tratcher added the breaking-change This issue / pr will introduce a breaking change, when resolved / merged. label Feb 14, 2019
Tratcher added a commit that referenced this issue Feb 16, 2019
* Disable AllowSynchronousIO by default in all servers
@Tratcher Tratcher added the Done This issue has been fixed label Feb 16, 2019
@Tratcher
Copy link
Member

#7644

@ghost ghost locked as resolved and limited conversation to collaborators Dec 4, 2019
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions blocked The work on this issue is blocked due to some dependency breaking-change This issue / pr will introduce a breaking change, when resolved / merged. Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-kestrel
Projects
None yet
Development

No branches or pull requests

7 participants