Skip to content

Call StopAsync before disposing #6189

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

Merged
merged 1 commit into from
Dec 30, 2018
Merged

Call StopAsync before disposing #6189

merged 1 commit into from
Dec 30, 2018

Conversation

davidfowl
Copy link
Member

@davidfowl davidfowl commented Dec 30, 2018

Turns out #6127 is caused by thread pool starvation that happens via calls to TestServer.Dispose, see #6127 (comment). I did this for the InMemory.FunctionalTests but there are likely other tests that need to be fixed. Ideally we would support IAsyncDisposable and would do await using instead of this once we get those features.

Without IAsyncDisposable, test failures could still cause this issue.

@davidfowl davidfowl requested review from halter73 and pakrym December 30, 2018 04:50
@pakrym
Copy link
Contributor

pakrym commented Dec 30, 2018

Why does StopAsync go async?

@davidfowl
Copy link
Member Author

@pakrym waiting for graceful shutdown. It waits 5 seconds by default for all connections to close https://github.com/aspnet/AspNetCore/blob/master/src/Servers/Kestrel/Core/src/KestrelServer.cs#L182-L205. In Kestrel, that means canceling input pipes and waiting for the task representing that connection to complete for all connections (usually one or 2 in tests).

@pakrym
Copy link
Contributor

pakrym commented Dec 30, 2018

Aren't we closing most of the connections before disposing the server?

@pakrym
Copy link
Contributor

pakrym commented Dec 30, 2018

This looks good as temporary solution before we get IAsyncDisposable. I just wonder why don't we get pass through shutdown when everything is in memory.

@davidfowl
Copy link
Member Author

Aren't we closing most of the connections before disposing the server?

Those get queued, so dispose queues shutdown to the thread pool for connections then returns. Then server shutdown occurs on the test thread itself while it waits for those callbacks to be executed while running on the thread pool. Even though it looks sequential, connection close and server shutdown happen in parallel.

This looks good as temporary solution before we get IAsyncDisposable. I just wonder why don't we get pass through shutdown when everything is in memory.

Yea, we need to switch over to that as soon as it becomes available. I'll file an issue. This change is too cumbersome but should make the tests generally more reliable.

@davidfowl davidfowl merged commit 010c1f0 into master Dec 30, 2018
@Tornhoof
Copy link
Contributor

@davidfowl Is there a reason why you call StopAsync in each method instead of using Xunit's IAsyncLifetime.DisposeAsync?
Or is IAsyncDisposable around the corner?

@davidfowl
Copy link
Member Author

@davidfowl Is there a reason why you call StopAsync in each method instead of using Xunit's IAsyncLifetime.DisposeAsync?

Wasn't aware of it but I will look at doing this in a few other places. I'm assuming this is for class level stuff. Most of these tests are creating and disposing things within each test and IAsyncDisposable fits that more naturally.

@Tornhoof
Copy link
Contributor

Yes, that's on Class Level. If you're still looking for starvation issues, RequestTests.TraceIdentifierIsUnique has Parallel.For in it.

@halter73 halter73 deleted the davidfowl/fix-blocking branch January 3, 2019 01:22
@BrennanConroy
Copy link
Member

@davidfowl Can we backport this to 2.2? Getting failures there https://dev.azure.com/dnceng/public/_build/results?buildId=72570

@davidfowl
Copy link
Member Author

@BrennanConroy sure 😄 Are you going to do it?

@BrennanConroy
Copy link
Member

Nope, looks like it might not even have fixed the issue? https://github.com/aspnet/AspNetCore-Internal/issues/1370#issuecomment-451596960

@davidfowl
Copy link
Member Author

It happens alot less, there might be other reasons for starvation but @JunTaoLuo is trying to collect a dump

@JunTaoLuo
Copy link
Contributor

Let's just back port the fix. If the test fails again, we'll be able to collect dumps. The test hasn't failed in a long time so @davidfowl 's fix certainly improved reliability.

@davidfowl
Copy link
Member Author

I'm not planning to do this so I suggest we file an issue, assign it to somebody so it doesn't get lost

@JunTaoLuo
Copy link
Contributor

@BrennanConroy seemed passionate so I volunteered him to backport.

JunTaoLuo pushed a commit that referenced this pull request Jan 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants