Support side-by-side transports in Kestrel #44657
Conversation
JamesNK
requested review from
Tratcher,
halter73 and
BrennanConroy
as code owners
JamesNK
changed the title
JamesNK
changed the title
JamesNK
force-pushed
the
jamesnk/kestrel-multiple-transports
branch
from
339ddb2 to
0e2b680
Compare
|
Updated to match API review. Please take a look 🙏 |
| @@ -1,6 +1,8 @@ | |||
| #nullable enable | |||
| Microsoft.AspNetCore.Connections.Features.IStreamClosedFeature | |||
| Microsoft.AspNetCore.Connections.Features.IStreamClosedFeature.OnClosed(System.Action<object?>! callback, object? state) -> void | |||
| Microsoft.AspNetCore.Connections.IConnectionListenerFactorySelector | |||
| Microsoft.AspNetCore.Connections.IConnectionListenerFactorySelector.CanBind(System.Net.EndPoint! endpoint) -> bool | |||
There was a problem hiding this comment.
@dotnet/aspnet-build It looks like we need to Update our PublicAPI baselines now that we're adding new APIs for .NET 8.
We'll have to be careful not to include IPNetwork.Parse/TryParse and RequestLocalizationOptions.CultureInfoUseUserOverride in the .NET 7 baseline. I think those are the only APIs we've added since branching for .NET 8.
There was a problem hiding this comment.
Once 7.0.0 ships, we'll update the baselines. But not until then. That'll involve changes in release/7.0 and copying the Shipped files into main, then patching things up to reflect the new APIs in Unshipped files.
Up to you whether you wait. Some patching / conflict resolution will be needed either way. |
|
So far, before this PR, the things we've added in .NET 8 have been new in their respective PublicAPI.txt files. I suspect this will make fixing those up relatively easier than doing it for this PR.
Can we do this now since were not adding any more new APIs to .NET 7? If not, does this mean will do this on November 8? |
It doesn't really matter -- as long as the Shipped files haven't been touched in main recently. The Unshipped ➡️ Shipped process moves everything currently in the 7.0 Unshipped files and empties them out. Then we'll check the 7.0 files into main and will have to manually update the Unshipped files.
We'll do it on the 8th because that's when the APIs truly have shipped. |
|
So, can I merge this? It sounds like it can be merged, but I want to double check. This PR dependency of other another, so I’d like to merge it. |
If someone approves the changes, they're fine w/ me. |
|
@halter73 Could you approve so I can merge? |
|
|
||
| private static bool CanBindFactory(EndPoint endPoint, IConnectionListenerFactorySelector? selector) | ||
| { | ||
| // By default, the last registered factory binds to the endpoint. |
There was a problem hiding this comment.
How do you ensure the last wins? Are you relying on the order of items in the DI injected Lists? I thought those lists where given in the order added, so you'd have to reverse it to get the last one first?
edit Nevermind, I found the Reverse call in KestrelServerImpl.
| /// This interface should be implemented by <see cref="IConnectionListenerFactory"/> and <see cref="IMultiplexedConnectionListenerFactory"/> | ||
| /// types that want to control want endpoint instances they can bind to. | ||
| /// </remarks> | ||
| public interface IConnectionListenerFactorySelector |
There was a problem hiding this comment.
Should we implement this on the socket transport and restrict it to known endpoints?
ghost
added this to the 
amcasey
added
area-networking
Fixes #44537
I didn't override the sockets or QUIC transports to have a CanBind implementation. I figure that they will always be present as the final fallback transports.