Skip to content
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.

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

Add new KeepAliveInterval option in QuicClientTransportOptions #3991

Merged
merged 5 commits into from
May 23, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ internal QuicMultiplexedListener(
DefaultCloseErrorCode = (int)MultiplexedConnectionCloseError.Aborted,
DefaultStreamErrorCode = 0,
IdleTimeout = quicTransportOptions.IdleTimeout,
#if NET9_0_OR_GREATER
KeepAliveInterval = Timeout.InfiniteTimeSpan, // the server doesn't send PING frames
#endif
ServerAuthenticationOptions = authenticationOptions,
MaxInboundBidirectionalStreams = options.MaxBidirectionalStreams,
MaxInboundUnidirectionalStreams = options.MaxUnidirectionalStreams
Expand Down
3 changes: 3 additions & 0 deletions src/IceRpc.Transports.Quic/QuicClientTransport.cs
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ public IMultiplexedConnection CreateConnection(
DefaultCloseErrorCode = (int)MultiplexedConnectionCloseError.Aborted,
DefaultStreamErrorCode = 0,
IdleTimeout = _quicTransportOptions.IdleTimeout,
#if NET9_0_OR_GREATER
KeepAliveInterval = _quicTransportOptions.KeepAliveInterval,
#endif
LocalEndPoint = _quicTransportOptions.LocalNetworkAddress,
RemoteEndPoint = endpoint,
MaxInboundBidirectionalStreams = options.MaxBidirectionalStreams,
Expand Down
24 changes: 21 additions & 3 deletions src/IceRpc.Transports.Quic/QuicTransportOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,11 @@ public record class QuicTransportOptions
{
/// <summary>Gets or sets the idle timeout. This timeout is used to monitor the transport connection health. If no
/// data is received within the idle timeout period, the transport connection is aborted.</summary>
/// <value>The idle timeout. Defaults to <see cref="TimeSpan.Zero" /> meaning that the underlying QUIC
/// implementation default will be used.</value>
public TimeSpan IdleTimeout { get; set; } = TimeSpan.Zero;
/// <value>The idle timeout. Defaults to 30 seconds. <see cref="TimeSpan.Zero" /> means "use the default value
/// provided by the underlying implementation".</value>
/// <remarks>The idle timeout is negotiated by QUIC during connection establishment. The actual idle timeout value
/// for a connection can be lower than the value you've set here.</remarks>
public TimeSpan IdleTimeout { get; set; } = TimeSpan.FromSeconds(30);
Copy link
Member Author

Choose a reason for hiding this comment

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

Changed the default from Zero to 30 seconds - which is the msquic default (= what Zero meant). It's clearer to spell out the actual default, and it's highly desirable to use the same default in all IceRPC implementation.

}

/// <summary>The options class for configuring <see cref="QuicClientTransport"/>.</summary>
Expand All @@ -22,6 +24,22 @@ public sealed record class QuicClientTransportOptions : QuicTransportOptions
/// establishment.</summary>
/// <value>The address and port to bind to. Defaults to <see langword="null" />.</value>
public IPEndPoint? LocalNetworkAddress { get; set; }

/// <summary>Gets or sets the interval at which the client sends QUIC PING frames to the server to keep the
/// connection alive.</summary>
/// <value>The keep-alive interval. Defaults to 15 seconds. <see cref="TimeSpan.Zero" /> means: use the default
/// value provided by the underlying implementation. <see cref="Timeout.InfiniteTimeSpan" /> means: never send
/// PING frames.</value>
/// <remarks>Unlike the idle timeout, the keep-alive interval is not negotiated during connection establishment. We
/// recommend setting this interval at half the negotiated idle timeout value to keep a healthy connection alive
/// forever - really until it's closed due to inactivity at the RPC level via the
/// <see cref="ConnectionOptions.InactivityTimeout" />.
/// This option is a stop-gap: ideally the .NET QUIC implementation would provide a way to automatically send PING
/// frames to keep the connection alive based on the negotiated idle timeout. See
/// <see href="https://github.com/microsoft/msquic/issues/3880" />.</remarks>
/// <remarks>This option is only implemented for .NET 9.0. With .NET 8.0, neither the client nor the server sends
/// PING frames to keep the connection alive.</remarks>
public TimeSpan KeepAliveInterval { get; set; } = TimeSpan.FromSeconds(15);
}

/// <summary>The options class for configuring <see cref="QuicServerTransport"/>.</summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public async Task Compress_request_payload(
new BrotliStream(outStream, CompressionMode.Decompress) :
new DeflateStream(outStream, CompressionMode.Decompress);
var decompressedPayload = new byte[4096];
await decompressedStream.ReadAsync(decompressedPayload);
await decompressedStream.ReadAtLeastAsync(decompressedPayload, _payload.Length);
Copy link
Member Author

Choose a reason for hiding this comment

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

These are tiny unrelated fixes for analyzer warnings introduced in .NET 9.0.

Assert.That(decompressedPayload, Is.EqualTo(_payload));
payloadWriter.Complete();
}
Expand Down
2 changes: 1 addition & 1 deletion tests/IceRpc.Compressor.Tests/CompressorMiddlewareTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public async Task Compress_response_payload(
new BrotliStream(outStream, CompressionMode.Decompress) :
new DeflateStream(outStream, CompressionMode.Decompress);
byte[] decompressedPayload = new byte[4096];
await decompressedStream.ReadAsync(decompressedPayload);
await decompressedStream.ReadAtLeastAsync(decompressedPayload, _payload.Length);
Assert.That(decompressedPayload, Is.EqualTo(_payload));
payloadWriter.Complete();
}
Expand Down
48 changes: 45 additions & 3 deletions tests/IceRpc.Quic.Tests/Transports/QuicIdleTimeoutTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public void FixtureSetUp()
/// timeout.</summary>
/// <remarks>The behavior shown by this test is not desirable for IceRPC: we would prefer QUIC or IceRPC's QUIC
/// wrapper to keep this connection alive when there is an outstanding request.
/// See https://github.com/icerpc/icerpc-csharp/issues/3353.</remarks>
/// See <see href="https://github.com/icerpc/icerpc-csharp/issues/3353" />.</remarks>
[Test]
public async Task Quic_connection_idle_after_idle_timeout([Values]bool configureServer)
{
Expand Down Expand Up @@ -60,14 +60,56 @@ public async Task Quic_connection_idle_after_idle_timeout([Values]bool configure
// Act / Assert
var startTime = TimeSpan.FromMilliseconds(Environment.TickCount64);

Assert.That(readResult.Buffer.ToArray(), Is.EqualTo(data));

Assert.That(
async () => await sut.Local.Input.ReadAsync().AsTask(),
Throws.InstanceOf<IceRpcException>().With.Property("IceRpcError").EqualTo(IceRpcError.ConnectionIdle));

Assert.That(
TimeSpan.FromMilliseconds(Environment.TickCount64) - startTime,
Is.GreaterThan(TimeSpan.FromMilliseconds(490)));

Assert.That(readResult.Buffer.ToArray(), Is.EqualTo(data));
}

/// <summary>Verifies the QUIC connection is kept alive by the keep alive interval.</summary>
[Test]
public async Task Quic_connection_kept_alive_by_keep_alive_interval()
{
// Arrange
var services = new ServiceCollection().AddQuicTest();

services.AddOptions<QuicServerTransportOptions>("server").Configure(
options => options.IdleTimeout = TimeSpan.FromMilliseconds(500));

services.AddOptions<QuicClientTransportOptions>("client").Configure(
options => options.KeepAliveInterval = TimeSpan.FromMilliseconds(250));

await using ServiceProvider provider = services.BuildServiceProvider(validateScopes: true);

var clientServerConnection = provider.GetRequiredService<ClientServerMultiplexedConnection>();
await clientServerConnection.AcceptAndConnectAsync();
using var sut = await clientServerConnection.CreateAndAcceptStreamAsync(bidirectional: true);

// Simulate a request
var data = new byte[] { 0x1, 0x2, 0x3 };
await sut.Local.Output.WriteAsync(data);
ReadResult readResult = await sut.Remote.Input.ReadAsync();

// Act/Assert

// Without the keep-alive interval, the idle timer aborts the connection after 500ms.
using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(2));
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 would be clear to write the response and close the stream after the delay.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, I reworked the test and removed the cancellation token source.

Note that with QUIC, canceling a read is fatal to the stream. You can't cancel reading and then start reading again. We may be doing the same in Slic.


#if NET9_0_OR_GREATER
Copy link
Member Author

Choose a reason for hiding this comment

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

This assumes the unit tests are built with the same version of .NET as the IceRpc.Transport.Quic assembly (a safe assumption).

Assert.That(
async () => await sut.Local.Input.ReadAsync(cts.Token),
Throws.InstanceOf<OperationCanceledException>());
#else
Assert.That(
async () => await sut.Local.Input.ReadAsync().AsTask(),
Throws.InstanceOf<IceRpcException>().With.Property("IceRpcError").EqualTo(IceRpcError.ConnectionIdle));
#endif

Assert.That(readResult.Buffer.ToArray(), Is.EqualTo(data));
}
}
Loading