Skip to content

Remove QuicTransportOptions.IdleTimeout #35202

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Remove QuicTransportOptions.IdleTimeout #35202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
891ba8d
Remove QuicTransportOptions.IdleTimeout
wtgodbe Aug 9, 2021
dafa63c
Remove Feature
wtgodbe Aug 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/Servers/Kestrel/Transport.Quic/src/Internal/QuicConnectionListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,11 @@ public QuicConnectionListener(QuicTransportOptions options, IQuicTrace log, EndP

quicListenerOptions.ServerAuthenticationOptions = sslServerAuthenticationOptions;
quicListenerOptions.ListenEndPoint = endpoint as IPEndPoint;
quicListenerOptions.IdleTimeout = options.IdleTimeout;
quicListenerOptions.MaxBidirectionalStreams = options.MaxBidirectionalStreamCount;
quicListenerOptions.MaxUnidirectionalStreams = options.MaxUnidirectionalStreamCount;
// Set Quic idle timeout to a conservative 10 minutes - we handle idle itemouts gracefully in the Kestrel layer
// based off of KestrelServerLimits.KeepAliveTimeout
quicListenerOptions.IdleTimeout = TimeSpan.FromMinutes(10);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

We're all in the shared framework.


_listener = new QuicListener(quicListenerOptions);

Expand Down
2 changes: 0 additions & 2 deletions src/Servers/Kestrel/Transport.Quic/src/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@
*REMOVED*~static Microsoft.AspNetCore.Hosting.WebHostBuilderMsQuicExtensions.UseQuic(this Microsoft.AspNetCore.Hosting.IWebHostBuilder hostBuilder) -> Microsoft.AspNetCore.Hosting.IWebHostBuilder
Microsoft.AspNetCore.Hosting.WebHostBuilderQuicExtensions
Microsoft.AspNetCore.Server.Kestrel.Transport.Quic.QuicTransportOptions
Microsoft.AspNetCore.Server.Kestrel.Transport.Quic.QuicTransportOptions.IdleTimeout.get -> System.TimeSpan
Microsoft.AspNetCore.Server.Kestrel.Transport.Quic.QuicTransportOptions.IdleTimeout.set -> void
Microsoft.AspNetCore.Server.Kestrel.Transport.Quic.QuicTransportOptions.MaxBidirectionalStreamCount.get -> ushort
Microsoft.AspNetCore.Server.Kestrel.Transport.Quic.QuicTransportOptions.MaxBidirectionalStreamCount.set -> void
Microsoft.AspNetCore.Server.Kestrel.Transport.Quic.QuicTransportOptions.MaxReadBufferSize.get -> long?
Expand Down
5 changes: 0 additions & 5 deletions src/Servers/Kestrel/Transport.Quic/src/QuicTransportOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,6 @@ public class QuicTransportOptions
/// </summary>
public ushort MaxUnidirectionalStreamCount { get; set; } = 10;

/// <summary>
/// Sets the idle timeout for connections and streams.
/// </summary>
public TimeSpan IdleTimeout { get; set; } = TimeSpan.FromSeconds(130); // Matches KestrelServerLimits.KeepAliveTimeout.

/// <summary>
/// The maximum read size.
/// </summary>
Expand Down
1 change: 0 additions & 1 deletion src/Servers/Kestrel/Transport.Quic/test/QuicTestHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ internal static class QuicTestHelpers
public static QuicTransportFactory CreateTransportFactory(ILoggerFactory loggerFactory = null, ISystemClock systemClock = null)
{
var quicTransportOptions = new QuicTransportOptions();
quicTransportOptions.IdleTimeout = TimeSpan.FromMinutes(1);
quicTransportOptions.MaxBidirectionalStreamCount = 200;
quicTransportOptions.MaxUnidirectionalStreamCount = 200;
if (systemClock != null)
Expand Down