Skip to content

Remove flaky HttpSys test Server_MultipleOutstandingSyncRequests_Success#11622

Merged
Tratcher merged 1 commit into
masterfrom
tratcher/flakyMultipleRequests
Jun 27, 2019
Merged

Remove flaky HttpSys test Server_MultipleOutstandingSyncRequests_Success#11622
Tratcher merged 1 commit into
masterfrom
tratcher/flakyMultipleRequests

Conversation

@Tratcher
Copy link
Copy Markdown
Member

https://github.com/aspnet/AspNetCore-Internal/issues/2415
Since switching to Arcade this has become the second flakiest test.
https://dev.azure.com/dnceng/public/_test/analytics?definitionId=278&contextType=build
It's intentionally trying to test sync thread blocking, but that makes it extremely vulnerable to thread pool starvation that we've experienced elsewhere in our tests. Even leaving the test here marked as flaky has a negative impact on other tests.

This test has limited value so I'm removing it.

@Tratcher Tratcher added this to the 3.0.0-preview7 milestone Jun 26, 2019
@Tratcher Tratcher self-assigned this Jun 26, 2019
@halter73
Copy link
Copy Markdown
Member

halter73 commented Jun 26, 2019

Could we move tests that need to block a bunch of threads to their own test group that doesn't run tests in parallel. Deleting every test that blocks a lot of threads doesn't seem sustainable. Sometimes tests simply have to block multiple threads to test a certain behavior.

I get that Server_MultipleOutstandingSyncRequests_Success might have "limited value", but I what about other tests that spawn several threads like Kestrel's ListenerPrimaryTests? Then it can be a tougher decision to delete the test. If we had a special test group to add it to, that'd be great.

Also, can you explain why this test is of limited value?

@Tratcher
Copy link
Copy Markdown
Member Author

This test existed only to prove that the message pump used multiple threads and that it would keep going even if the app blocked threads. But what we're seeing now is that the server really can't handle threadpool exhaustion so there's no point in testing that it can. There's an async version of this test that works fine.

@halter73
Copy link
Copy Markdown
Member

But what we're seeing now is that the server really can't handle threadpool exhaustion so there's no point in testing that it can.

I don't think this is a reason to stop testing that our servers can support at least a small number of blocked request threads. A lot of real customer apps block, and it often isn't too much of a problem as long as there isn't too much load.

@davidfowl
Copy link
Copy Markdown
Member

I think the test is fine to delete

@Tratcher Tratcher merged commit fd9ad7f into master Jun 27, 2019
@ghost ghost deleted the tratcher/flakyMultipleRequests branch June 27, 2019 05:18
@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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants