Remove QuicTransportOptions.IdleTimeout

[wtgodbe]

Fixes #34955

[wtgodbe]

Need to update

aspnetcore/src/Servers/Kestrel/Core/src/Internal/Infrastructure/TransportManager.cs

Lines 68 to 72 in fd65bd8

	features.Set(sslServerAuthenticationOptions);
	}
	var transport = await _multiplexedTransportFactory.BindAsync(endPoint, features, cancellationToken).ConfigureAwait(false);

to set the feature (probably listenOptions.KestrelServerOptions.Limits.KeepAliveTimeout). @JamesNK @Tratcher I'm assuming the feature class should be internal? If so, where do we think it should live, given that we're using it in Kestrel.Core & Transport.Quic. Kestrel.Core + IVT?

[Tratcher]

The feature should be considered public API in the Quic Transport library, it's just a different way of passing options.

[halter73]

Where does this get set in real life outside of tests? Does this need to be set in TransportManager or something?

[JamesNK]

Add an interop test (these are in Http3RequestTests.cs in Interop.FunctionalTests project) that creates a server with a very low timeout, e.g. 1 second, sends a request, and the request should fail when the timeout occurs.

[wtgodbe]

Ok @halter73 @Tratcher I updated this to remove the Feature & just set the timeout to 10 mins in QuicConnectionListener

[JamesNK]

KeepAliveTimeout isn't implemented in Kestrel layer for HTTP/3. We're using the QUIC idle timeout.

[halter73]

Is there a reason not to hard-code this to Kestrel's default 130 second KeepAliveTimeout?

[Tratcher]

It's not configurable so we might want to be more generous?

[halter73]

Given the pushback I got increasing this default by 10 seconds, I'm surprised we'd want to increase it nearly five-fold for Quic while making it non-configurable. Why is making it more generous better than making it more conservative? I'd just keep the default.

[JamesNK]

Why aren't we making it configurable? I thought the idea here was to make IdleTimeout = KestrelServerLimits.KeepAliveTimeout

[wtgodbe]

Stephen's argument earlier was:

If we want to continue exposing QuicTransportOptions.IdleTimeout fine, but I don't think we have to because it is redundant with the KeepAliveTimeout and the shorter one would win.

Which I buy. Having the extra API surface in the form of the Feature doesn't seem fully necessary. As for default vs. 10 min, am I misunderstanding that the shorter one would win today? If that's the case it doesn't seem like it would matter, so maybe it's cleaner to have them use the same 130 second default.

[halter73]

I think the better layering is to have Kestrel.Core track the idleness of the connection and close the connection manually after the KeepAliveTimeout like we do for non-multiplexed transports rather than require the transport fish out a feature passed to BindAsync and implement a KeepAliveTimeout/IdleTimeout itself. We figured doing this might be a bit too much work for .NET 6, but adding a new public API just to remove it in the next release also feels bad, so we ended up here. See #35202 (comment).

[JamesNK]

Spicy: Make QuicTransportOptions.IdleTimeout internal. Kestrel sets it to KeepAliveTimeout.

We're all in the shared framework.

[wtgodbe]

Closing in favor of #35307