Add StopListeningAsync() to ConnectionListener

[halter73]

Background and Motivation

When migrating Kestrel to the new System.Net.Connections APIs, I noticed that there's no built-in ability to tell a listener to stop listening prior to disposing the transport.

We want this capability in Kestrel so new connections aren't accepted into the listen backlog as it is draining connections during shutdown. AFAIK, simply not calling AcceptAsync is not enough to prevent connections from being accepted into the listen backlog. Kestrel doesn't want to dispose the transport entirely before connections are drained because the transport owns the memory pool being used by the draining connections, and this memory pool is disposed with the transport.

Proposed APIs

namespace System.Net.Connections
{
    public class ConnectionListener
    {
+      public abstract ValueTask StopListeningAsync(CancellationToken cancellationToken = default);
    }

We can consider making this async, but I don't see why that would be necessary. It doesn't need to be async the way it's used today in Kestrel's Socket-based transport.

cc @geoffkizer @scalablecory @davidfowl

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

[scalablecory]

Seems reasonable. @geoffkizer anything to add before moving to API review?

[geoffkizer]

I'm trying to understand what user functionality this adds beyond just disposing the object. Is there anything?

If you call StopListeningAsync and then call AcceptAsync again, what exception do you get? ObjectDisposedException? Something else? If something else, then what? And if something else, that seems to imply that every Connection needs to track "stopped" differently than "disposed", which doesn't seem ideal.

If you call StopListeningAsync, can you then call ListenAsync again and start listening again? Presumably on a potentially new EndPoint? We actually had a long-standing bug in TcpListener where calling Stop and then Start didn't work in some cases. This is the kind of thing that no one ever does because it's not really useful, until someone decides to do it, and then you find out it doesn't really work properly.

If we are adding StopListeningAsync, then should we rename ListenAsync to StartListeningAsync?

[geoffkizer]

Kestrel doesn't want to dispose the transport entirely before connections are drained because the transport owns the memory pool being used by the draining connections, and this memory pool is disposed with the transport.

I'm certainly sympathetic to making the Kestrel porting effort easier, but it seems like this is something you could handle pretty easily by just adding something like KestrelConnection that holds both a Connection and the memory pool stuff, with a Stop method that disposes the underlying Connection but not the memory pool. Am I missing something?

[davidfowl]

The problem is that the listener factory currently owns both the listen socket and the memory pool and there's no way to say dispose/stop the listen socket without disposing the memory pool as well. We need a way to decouple the thing that is listening from any other resources that may be affecting existing connections.

[davidfowl]

If we are adding StopListeningAsync, then should we rename ListenAsync to StartListeningAsync?

I like ListenAsync but it doesn't have a good inverse (UnListen 😄 )

[halter73]

but it seems like this is something you could handle pretty easily by just adding something like KestrelConnection that holds both a Connection and the memory pool stuff,

Instead of sharing the memory pool ownership between the connections with some sort of reference counting, I've moved the memory pool ownership to the ConnectionListenerFactory instead of the ConnectionListener itself (I think @geoffkizer might have also suggested exactly this in our Teams chat). This allows Kestrel to dispose the listener before draining all the connections. It looks like this works well, so I'm closing this issue.