-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
[SignalR] Move to generic host #22602
Conversation
src/SignalR/clients/ts/FunctionalTests/SignalR.Client.FunctionalTestApp.csproj
Outdated
Show resolved
Hide resolved
Looks good. Still need to fix the ref assembly? |
Yeah. And I'm still trying to figure out the issue with close. Wont remove from draft until I've figured that out. |
finally | ||
{ | ||
await _host.StopAsync(); | ||
_host.Dispose(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BrennanConroy and I learned by investigating test failures related to this change that unlike WebHost.Dispose(), the generic Host.Dispose() just disposes all services.
Even when configured with ConfigureWebHost, Host.Dispose() does not fire ApplicationStopping, ApplicationStopped or call IServer.StopAsync(). Instead KestrelServer just gets disposed which completely bypasses graceful shutdown and aborts all connections. Naturally, this lead to DiagnosticMemoryPool errors in the tests.
I think we should make Host.Dispose() gracefully shutdown the server like WebHost.Dispose() did. Should I file an issue for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to open an issue for triage, it's out of scope for this PR.
I don't think Dispose should be graceful, that's why we added Stop and helpers like Run that call Stop and Dispose for you. I think this was a conscious choice back in 2.2 or 3.0, if not widely discussed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was in no way suggesting we change the generic host behavior in this PR. I'm just gauging whether it's worthwhile to even create an issue. It doesn't sound like we're too concerned. Having Dispose() be abortive does make it less sync-over-async which is nice.
src/SignalR/clients/ts/FunctionalTests/SignalR.Client.FunctionalTestApp.csproj
Outdated
Show resolved
Hide resolved
…alTestApp.csproj Co-authored-by: Chris Ross <Tratcher@Outlook.com>
I don't think I can merge this :/ For some reason it makes the issue at dotnet/runtime#30056 happen occasionally and we're trying to reduce overall flakiness, not introduce more! |
Ran this multiple times on Mac after changing to |
|
Part of #20964
Easier to review with whitespace turned off