[release/5.0] Allow ConnectionListener.AcceptAsync() to return null

[github-actions]

Backport of #41442 to release/5.0

As it was discussed in #41304, with current System.Net.Connections APIs, listeners need to throw from AcceptAsync when shutting down since AcceptAsync cannot return null. This PR allows AcceptAsync to return null, to avoid unnecessary first-chance exceptions when listeners are gracefully shutting down.

Customer Impact

It's all about usability. A server will always have a loop accepting new connections, until the server is shutdown. So, handling that via returning null is far better than throwing an exception -- we shouldn't make handling an exception part of the standard non-error path.

Testing

N/A

Risk

There is no implementation, and we are only relaxing nullability. Risk is low.

[ghost]

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

[karelz]

I support this change to be ported to 5.0 -- it is feedback from adoption in Kestrel, which is blocking them to achieve their goals.

[karelz]

Ready for your approval @ericstj

[ericstj]

Do we have any implementations of this abstract class in runtime or aspnetcore that need to react to this change? Are they prepared to react to this in time for RC1?

[karelz]

Yes, @halter73 from ASP.NET team is ready to react to this change in RC1.

[geoffkizer]

There's test code here that implements this API:

runtime/src/libraries/Common/tests/System/Net/VirtualNetwork/VirtualNetworkConnectionListenerFactory.cs

Line 56 in 8b5467e

public override async ValueTask<Connection> AcceptAsync(IConnectionProperties options = null, CancellationToken cancellationToken = default)

This code is used in Connection tests, possibly elsewhere.

I assume this didn't break because it's just a nullability change, but we should fix it regardless. And we should fix the behavior of the test code to return null when disposed and validate this.

[karelz]

Good point @geoffkizer -- I don't think this has to be blocking 5.0 merge though, would you agree?
We can do it in 6.0 branch as test-only change and then consider porting it separately into 5.0 (depends on the bar).

[geoffkizer]

I'm fine with that.

[karelz]

@ericstj it's ready for your approval now ...

[ericstj]

Thanks for addressing my concerns around reacting to the change.