Add support for customising the creation of Kestrel listen sockets

[deanward81]

Fixes #32794. As mentioned in the issue it is currently not possible to configure the underlying listen socket used by Kestrel. In certain circumstances it is desirable to be able to configure socket options when the socket is created - a concrete case that I recently came across is setting the SO_RECV_ANYIF socket option on macOS so that Kestrel can listen on the awdl0 interface (more details on that specific case here).

This change adds the API suggested by #32794 by adding a function that allows the caller to manage the creation of the listening socket - Func<EndPoint, Socket> CreateListenSocket { get; set; } allows the creation of the listen socket to be configured.

There's also some tests using IPv4, IPv6, unix domain sockets and file handles.

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[halter73]

I wonder if it wouldn't be better to change ConfigureListenSocket from Action<EndPoint, Socket> to Func<EndPoint, Socket>. If we went this route, we'd want to expose a Socket CreateListenSocket(EndPoint) method somewhere and document it with these callbacks.

[Tratcher]

Create seems like a different scenario from augmentation, and requires more from the implementer.

[halter73]

If we go the Action<EndPoint, Socket> route, wouldn't it be better to call it after binding but before listening? This would make the FileHandleEndPoint and IPEndPoint cases more similar.

If we go the Func<EndPoint, Socket> route, I'd expect the returned Socket to already be bound.

[halter73]

and requires more from the implementer.

Not really given Socket CreateListenSocket(EndPoint) exists.

[deanward81]

and requires more from the implementer.

Not really given Socket CreateListenSocket(EndPoint) exists.

Are you saying that this method would exist when moving to a Func? I can’t see anything existing like this method except CreateListenSocket in libuv that returns a UvStreamHandle:

aspnetcore/src/Servers/Kestrel/Transport.Libuv/src/Internal/Listener.cs

Line 49 in 52eff90

private UvStreamHandle CreateListenSocket()

[halter73]

New proposed API discussed in triage:

public class SocketTransportOptions
{
+    public Func<EndPoint, Socket> CreateListenSocket { get; set; }
+    public static Socket CreateDefaultListenSocket(EndPoint);
}

We don't think ConfigureAcceptedSocket adds much value. We recommend adding connection middleware and using the new IConnectionSocketFeature (#31588).

[davidfowl]

@halter73 how do you "call base" for the listen socket.

[Tratcher]

Is CreateDefaultListenSocket supposed to be static?

[davidfowl]

Yea it should be the stateless default.

[halter73]

Yeah. It should be static. I think it would basically be the current implementation SocketConnectionListen.Bind(), but instead of calling Socket.Listen(int) and setting _listenSocket, it would return the Socket before listening. The new implementation of Bind() would then call the SocketTransportOptions.CreateListenSocket(EndPoint) Func and then call Socket.Listen(int) on the returned value and set it to _listenSocket.

We might have to bypass the CreateListenSocket for FileHandleEndPoints since I can't think of a great way to manage the _socketHandle in that case. I think this is fine because if you're using a FileHandleEndPoint, you can configure the socket options before passing it to KestrelServerOptions.ListenHandle(long).

[halter73]

If we make it non-static, we could do some trickery where the SocketConnectionListener adds an internal reference to itself to SocketTransportOptions before calling the CreateDefaultListenSocket callback allowing CreateDefaultListenSocket to set _socketHandle if need be. Not sure if this would be worthwhile, but I do like that it makes it work in all cases.

We could also make CreateListenSocket a Func<EndPoint, Func<EndPoint, Socket>, Socket> and do away with CreateDefaultListenSocket.

[deanward81]

We might have to bypass the CreateListenSocket for FileHandleEndPoints since I can't think of a great way to manage the _socketHandle in that case. I think this is fine because if you're using a FileHandleEndPoint, you can configure the socket options before passing it to KestrelServerOptions.ListenHandle(long).

Does the _socketHandle field need to be there? It's only used during disposal, but that handle is created with ownsHandle: true which, when passed to Socket, means the handle is cleaned up in the finaliser of the socket (which arguably is more correct because it's an unmanaged resource). https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs#L3262

That said, is the handle really owned in this case? The user is passing an opaque handle created elsewhere - is it intentional that it is marked as "owned" by the socket transport? Shouldn't they be responsible for cleanup?

Reason I ask - not having that field allows CreateDefaultListenSocket to work for all cases and keeps things decoupled (no need to do mess with references to the SocketConnectionListener in the instance case).

[deanward81]

I've updated to use the triaged API (using a static CreateDefaultListenSocket). I removed the _socketHandle in the last commit and, so far as I can tell, there are no negative consequences (tests pass, at least). Details (from the commit):

Note that during implementation I removed the _socketHandle field from SocketConnectionListener - this was only set so that Dispose could be called when the listener is disposed. Under the hood Socket already disposes a handle passed to it during finalization, but only if the ownsHandle parameter is true . In this case the SafeSocketHandle is instantiated with this parameter so the the underlying handle will be closed when the _listenSocket field is disposed - that is currently the case when the listener is disposed or unbound.

[halter73]

New new proposed API based on #32827 (comment) 😆

public class SocketTransportOptions
{
+    public Func<EndPoint, Func<EndPoint, Socket>, Socket> CreateListenSocket { get; set; }
}

Edit: never mind

[halter73]

I now see that the SafeSocketHandle is disposed by Socket all along because we were specifying ownsHandle: true. This means the previous manual tracking of the SafeSocketHandle was redundant and there's no behavioral change. Or at least that's what it seems like. I'll step through the code to confirm before merging.

[JamesNK]

@halter73 Static method is new as well for API review?

[halter73]

Yes. This is still api-ready-for-review. We shouldn't merge before it's approved.

[BrennanConroy]

API review:

public class SocketTransportOptions
{
+     public static Socket CreateDefaultBoundListenSocket(EndPoint);
+     public Func<EndPoint, Socket> CreateBoundListenSocket { get; set; }
}

[deanward81]

@BrennanConroy currently the implementation of CreateDefaultListenSocket does not call Bind - the connection listener does it - with the approved API it implies that it should be called - presumably the implementation should be changed to do so?

[deanward81]

I've updated the code to match the API review. Let me know if you want any additional tweaks @BrennanConroy

[davidfowl]

@BrennanConroy @halter73 is this good?

[halter73]

Thanks @deanward81!

[sebastienros]

Did we measure the perf impact of that change? Asking for a friend :/

[halter73]

@sebastienros We didn't measure the perf of this change, but I'd be shocked if this had a perf impact. This moves the creation of the listen socket (not the accepted socket) to a Func, but keeps the logic the same by default. Even if there was a perf hit from calling a Func, it's only called once per address at app startup.

[halter73]

#34769 would be more likely to affect Connection: close perf because it touches a per-connection code path. I don't think we explicitly perf tested that. However, it's basically just a refactoring, so it shouldn't impact perf.