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

Disable AllowSynchronousIO by default in all servers #5120

Merged
merged 2 commits into from
Feb 16, 2019
Merged

Conversation

Tratcher
Copy link
Member

@Tratcher Tratcher commented Dec 14, 2018

#4774 #5874
I had to bump IIS's dependencies to get the response compression fix. This will go away when I can rebase on the refs changes.

Let's see how many upstream tests fail...

Blocked by MVC #6397.

@Tratcher Tratcher added this to the 3.0.0-preview2 milestone Dec 14, 2018
@Tratcher Tratcher self-assigned this Dec 14, 2018
@Tratcher
Copy link
Member Author

Rebase on #4962

@Tratcher
Copy link
Member Author

Ready for review. The failures are known logging issues @pakrym is dealing with.

@Tratcher
Copy link
Member Author

Hmm, this should have caused more failures, our test code isn't that clean...

I'm going to add the same feature to the test server and see what happens.

@NinoFloris
Copy link

What's left of the ambition to expose the pipes instead of sticking with streams?

@Tratcher
Copy link
Member Author

@NinoFloris that's still happening. #4757

@Tratcher
Copy link
Member Author

2018-12-20T00:53:00.2990733Z   Failed   Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests.CompressionModuleTests.DynamicResponsesAreCompressed
2018-12-20T00:53:00.2991617Z   Error Message:
2018-12-20T00:53:00.3000411Z    Assert.Equal() Failure
2018-12-20T00:53:00.3007589Z                                     � (pos 20)
2018-12-20T00:53:00.3007782Z   Expected: Message1\r\nMessage2\r\n
2018-12-20T00:53:00.3012128Z   Actual:   Message1\r\nMessage2\r\n\r\n
2018-12-20T00:53:00.3015538Z                                     � (pos 20)

All other failures look like known unrelated issues.

@davidfowl
Copy link
Member

What's left of the ambition to expose the pipes instead of sticking with streams.

There are a bunch of work items for doing this in the 3.0 milestone. We think we have a plan that is both backwards compatible and exposes pipes with streams

@Tratcher
Copy link
Member Author

Tratcher commented Jan 4, 2019

Blocked by MVC #6397. @davidfowl how about I turn the setting back off by default but merge the other improvements I've made so far like the test fixes, the new test host feature, etc.?

@Tratcher Tratcher changed the base branch from master to tratcher/nosyncfixes January 4, 2019 22:01
@Tratcher
Copy link
Member Author

Tratcher commented Jan 4, 2019

Split. #6404 deals with the improvements so far and this PR only deals with changing the defaults.

@Tratcher Tratcher added the blocked The work on this issue is blocked due to some dependency label Jan 4, 2019
@davidfowl
Copy link
Member

The other thing we can do is turn it off in those code paths to keep everything clean.

@Tratcher
Copy link
Member Author

Tratcher commented Jan 4, 2019

I doubt that would be clean. Hundreds of MVC tests are affected on many different code paths.

@davidfowl
Copy link
Member

I mean no new broken logic would sneak in if we can turn it on globally and enable sync IO on those code paths.

@Tratcher Tratcher force-pushed the tratcher/nosync branch 2 times, most recently from 01848f4 to 4880d2d Compare January 15, 2019 18:47
@Tratcher Tratcher changed the base branch from tratcher/nosyncfixes to master January 15, 2019 19:49
@Tratcher Tratcher force-pushed the tratcher/nosync branch 2 times, most recently from f62a8e8 to 4a8cb9e Compare February 13, 2019 19:27
@Tratcher Tratcher added the breaking-change This issue / pr will introduce a breaking change, when resolved / merged. label Feb 13, 2019
@Tratcher Tratcher removed the blocked The work on this issue is blocked due to some dependency label Feb 15, 2019
@Tratcher
Copy link
Member Author

Ready for a final review. I've added some fixes and workarounds in MVC, and cleaned up a few more tests elsewhere.

(The Helix failures are unrelated)

Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double approved.

@Tratcher Tratcher merged commit 93a24b0 into master Feb 16, 2019
@Tratcher Tratcher deleted the tratcher/nosync branch February 16, 2019 00:05
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions breaking-change This issue / pr will introduce a breaking change, when resolved / merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants