When using a ConnectionHandler, socket is left open after Kestrel close

[mconnew]

Describe the bug

AspNetCore 2.1 doesn't automatically close the socket when a ConnectionHandler's OnConnectedAsync returned Task completes. On AspNetCore 2.2, the socket is automatically closed on Task completion. If the connection handler implementation never closes the socket, then the socket remains open indefinitely, even after Dispose has been called on the IWebHost.

To Reproduce

Steps to reproduce the behavior:

1. Using this version of ASP.NET Core '2.1'
2. Create a connection handler which returns a completed Task. Start the webhost and create a connection to the configured port. Dispose the webhost. Clients socket remains connected and is able to call send and receive on the socket (which don't complete as there's nothing reading or writing on the server side of the socket).

Expected behavior

When calling Dispose on the webhost, all sockets will get disconnected.

Additional context

I discovered this when developing CoreWCF. If you need a repro, I can make a custom branch where our unit tests fail demonstrating this issue.

[halter73]

Discussing this in triage, there are not really many options in 2.1. In 3.0 2.2, we did a major refactoring to track any ConnectionContext and not just HTTP connections in Kestrel. I doubt we could backport this. Maybe you could replace Hosting's sigterm/ctrl+c event handlers? I feel bad just typing that.

[mconnew]

If the process is exiting (as is the case with sigterm/ctrl+c), then the sockets will get closed on process exit. The issue is when the WebHost closes and the process continues to run.

[davidfowl]

It’s also easy to work around. Here was the change in 2.2 aspnet/KestrelHttpServer#2735

[halter73]

It’s also easy to work around. Here was the change in 2.2 aspnet/KestrelHttpServer#2735

Manually closing the connection is easy. What might not be so easy is delaying server shutdown, and therefore the disposal of the memory pool until all the ConnectionHandler's have completed. In 2.2, Kestrel only tracked HTTP connections, and it's not pluggable AFAICT.

[davidfowl]

Ah triggering abort on shutdown yes that was a big change

[mconnew]

This is actually a feature that WCF has on .NET Framework. Our ServiceHost has a configured CloseTimeout, which can also be overridden when calling Close/BeginClose. WCF then iterates through each active connection and attempts to Close them. When each connection has Close called, if there's an ongoing request, it uses whatever timeout time is now left to wait for the request to complete (response sent), then it's closed. When the timeout is reached, if there are any connections which haven't been closed yet, all remaining ones are aborted.
If you add a mechanism to delay server shutdown, there needs to be two phases. First a clean shutdown phase where the ConnectionHandler attempts to clean everything up nicely, then a phase where the ConnectionHandler aborts everything. You can't pull the rug out from underneath it after the first phase as there might be Aborted event handlers which need to run and yanking the connection might cause that to fail. Although I can see an argument for there being an abort time limit too where the ConnectionHandler is expected to abort everything within that time otherwise things will be yanked out from beneath.

[davidfowl]

Yes Kestrel already does this for HTTP connections in 2.1 and it became more general purpose in 2.2 with this change (aspnet/KestrelHttpServer#2834). It's not something we can backport to 2.1 since the change is big and requires new APIs (in particular the IConnectionLifetimeNotificationFeature).