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

Always flush headers on first response write (#1202). #1204

Merged
merged 2 commits into from
Nov 9, 2016
Merged

Conversation

halter73
Copy link
Member

@halter73 halter73 commented Nov 8, 2016

"",
"");

flushed.Release();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👍

@cesarblum
Copy link
Contributor

Check AppVeyor before merging.

@muratg
Copy link
Contributor

muratg commented Nov 8, 2016

@halter73 did you investigate the appveyor failure? otherwise is it good to check-in? I expect @pranavkm to try to get the build today.

using (var server = new TestServer(async httpContext =>
{
httpContext.Response.ContentLength = 12;
await httpContext.Response.WriteAsync("hello, world");
await flushed.WaitAsync();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add reasonable timeouts to the semaphore Waits?

Copy link
Member Author

Choose a reason for hiding this comment

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

These calls to WaitAsync() are in the web app on the test. If the web app hangs, the test will not hang. Instead the test will fail when it times out waiting for a response. The response timeout here is 1 minute like for most of our tests.

@halter73
Copy link
Member Author

halter73 commented Nov 9, 2016

When the failing test (ThrowingSynchronousConnectionFilterDoesNotCrashServer) is run individually (using dotnet test -method ...) on the dev branch it fails in the same way it did on appveyor. It seems to just be a flaky test. I'm modifying the test to make it more reliable.

@halter73
Copy link
Member Author

halter73 commented Nov 9, 2016

@pranavkm Are you fine with me merging this?

@pranavkm
Copy link
Contributor

pranavkm commented Nov 9, 2016

:shipit:

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants