[Kestrel] Move to GenericHost

[Kahbazi]

Part of #20964

[Tratcher]

Lots of test failures.
https://dev.azure.com/dnceng/public/_build/results?buildId=744551&view=ms.vss-test-web.build-test-results-tab&runId=23076038&resultId=103605&paneView=debug

[Kahbazi]

I'm having a difficult time fixing the tests, I think the reason is the different implementation of Dispose method between Host and WebHost.
When WebHost is being disposed, it will also call StopAsync() which also stops the IServer,
but when Host is being disposed, it won't call StopAsync().
Is this behavior by design?
Am I on the right path?

[Tratcher]

Yes, it's different because StopAsync was added to the host later. The tests should call StopAsync.

[Kahbazi]

I still couldn't pass ServerShutsDownGracefullyWhenMaxRequestBufferSizeExceeded and GracefulTurnsAbortiveIfRequestsDoNotFinish. It looks like it hangs here until host Shutdown timeout.

aspnetcore/src/Servers/Kestrel/Core/src/Internal/Infrastructure/TransportConnectionManager.cs

Line 88 in 49aecc3

return await Task.WhenAny(allAbortedTask, Task.Delay(1000)).ConfigureAwait(false) == allAbortedTask;

[BrennanConroy]

Looks like a few tests are still failing because StopAsync isn't being called.

[BrennanConroy]

I'm not sure about the other test failure. Might have to take a closer look in a bit.

[halter73]

We should make TestServer implement IAsyncDisposable, so we can just change our usings to await usings. Even though IHost doesn't implement IAsyncDisposable, Host does so we can just cast.

Surprisingly, it doesn't look like disposing the Host actually stops it when it's running, but I haven't tested that. Not a big deal either way as we can always call Host.StopAsync() before calling Host.DispooseAsync() in TestServer.DisposeAsync().

[Kahbazi]

Should I add Host.StopAsync() in TestServer.DisposeAsync()? Right now I call server.StopAsync() for every instance of TestServer before it is disposing.

[BrennanConroy]

That's what we did in SignalR tests. It would clean up all the tests a little :D

[Kahbazi]

Ok. Time to revert some commits 😁 I'll remove the server.StopAsync from the tests.

[halter73]

It doesn't have to be done for this PR, but long term I'd like to get rid of all calls to the IHost.Start() and IHost.Dispose() in our tests and instead use their async alternatives. This will mean moving away from using the TestServer constructor to give as an already-started TestServer. We might need to switch to an async TestServer factory method instead.

[BrennanConroy]

Took a look at the ServerShutsDownGracefullyWhenMaxRequestBufferSizeExceeded test.

It looks like close is working the same as before, where the server shutdown is closing ungracefully because of the blocking app code, the only difference between WebHost and Host is that after the server shutdown there is a cancellation token check in Host which is causing the exception, in WebHost this check didn't exist.

https://github.com/dotnet/runtime/blob/df7f985f44712efd46b8b3bc5e4d83fe3d83ed4b/src/libraries/Microsoft.Extensions.Hosting/src/Internal/Host.cs#L91

the server shutdown is closing ungracefully

The test name seems to indicate there should be a graceful close, so either the test name isn't quite accurate, or the server code has changed since this test was made and there isn't really a check for graceful close in this test.

@Tratcher @halter73 Thoughts on this test? Should we just put a OperationCanceledException catch around Host.StopAsync, or maybe stop the server explicitly instead of the Host to verify it doesn't throw?

[halter73]

Regarding ServerShutsDownGracefullyWhenMaxRequestBufferSizeExceeded. It's too bad we're just now noticing the difference in behavior for ungraceful shutdown with the generic Host.

Kestrel has always tried to treat the token being canceled during KesterlServer.StopAync() as gracefully as possible, since long running requests/connection that aren't listening to the stopping notification are not that uncommon. Kestrel writes a debug log, but that's it. It doesn't throw which would cause most apps to exit with an exception.

I think we might need to fix the runtime to just remove the call to token.ThrowIfCancellationRequested(). I'd consider this a regression since WebHost doesn't do this. Not only that, but that call to token.ThrowIfCancellationRequested() prevents the following call to _hostLifetime.StopAsync(token) from ever happening. Fortunately ConsoleLifetime.StopAsync(CancellationToken) currently doesn't do anything, but what if it did?

[halter73]

Basically the semantics when passing a canceled token (or a token that becomes canceled) to KestrelServer.StopAsync has always been to do an abortive shutdown without throwing, but then generic host is treating a canceled token as indicating StopAsync can be completely skipped (at least for the IHostedLifetime) and StopAsync should throw. Generic host might be using the more common semantics for CancellationToken, but we need to be using Kestrel's semantics if we're going to use the CancellationToken to indicate shutdown should become abortive (rather than to indicate shutdown should be skipped) and I don't see any alternative this late before 5.0.

[Tratcher]

@halter73 lets talk after triage. People have been using Kestrel this way since 3.0, we shouldn't be changing the host's behavior now.

[ghost]

Hello @BrennanConroy!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.