From b966b21ef5d34d738f68d92d067458ddf3e92388 Mon Sep 17 00:00:00 2001 From: Bernard Normier Date: Sat, 16 Nov 2024 11:40:59 -0500 Subject: [PATCH 1/3] Update client IceProtocolConnection to schedule first heartbeat only once connected --- .../Internal/IceDuplexConnectionDecorator.cs | 17 ++++----- src/IceRpc/Internal/IceProtocolConnection.cs | 10 ++++- .../IceProtocolConnectionTests.cs | 37 +++++++++++++++++++ 3 files changed, 52 insertions(+), 12 deletions(-) diff --git a/src/IceRpc/Internal/IceDuplexConnectionDecorator.cs b/src/IceRpc/Internal/IceDuplexConnectionDecorator.cs index d26885565..2d9eac39a 100644 --- a/src/IceRpc/Internal/IceDuplexConnectionDecorator.cs +++ b/src/IceRpc/Internal/IceDuplexConnectionDecorator.cs @@ -17,15 +17,8 @@ internal class IceDuplexConnectionDecorator : IDuplexConnection private readonly TimeSpan _writeIdleTimeout; private readonly Timer _writeTimer; - public async Task ConnectAsync(CancellationToken cancellationToken) - { - TransportConnectionInformation connectionInformation = await _decoratee.ConnectAsync(cancellationToken) - .ConfigureAwait(false); - - // Schedule or reschedule a heartbeat after a successful connection establishment. - RescheduleWriteTimer(); - return connectionInformation; - } + public Task ConnectAsync(CancellationToken cancellationToken) => + _decoratee.ConnectAsync(cancellationToken); public void Dispose() { @@ -104,9 +97,13 @@ internal IceDuplexConnectionDecorator( _writeIdleTimeout = writeIdleTimeout; _writeTimer = new Timer(_ => sendHeartbeat()); - // We can't schedule a keep alive right away because the connection is not connected yet. + // We can't schedule a heartbeat right away because the connection is not connected yet. } + /// Schedules the initial heartbeat. Called by a client IceProtocolConnection once it receives the + /// initial ValidateConnection frame from the server. + internal void ScheduleHeartbeat() => RescheduleWriteTimer(); + /// Cancels the write timer. private void CancelWriteTimer() => _writeTimer.Change(Timeout.InfiniteTimeSpan, Timeout.InfiniteTimeSpan); diff --git a/src/IceRpc/Internal/IceProtocolConnection.cs b/src/IceRpc/Internal/IceProtocolConnection.cs index d298bb022..dd276912c 100644 --- a/src/IceRpc/Internal/IceProtocolConnection.cs +++ b/src/IceRpc/Internal/IceProtocolConnection.cs @@ -117,8 +117,8 @@ internal sealed class IceProtocolConnection : IProtocolConnection // Send ValidateConnection frame. await SendControlFrameAsync(EncodeValidateConnectionFrame, connectCts.Token).ConfigureAwait(false); - // The SendControlFrameAsync is a "write" that schedules a keep-alive when the idle timeout is not - // infinite. + // The SendControlFrameAsync is a "write" that schedules a heartbeat when the idle timeout is not + // infinite. So no need to call scheduleHeartbeat. } else { @@ -140,6 +140,12 @@ internal sealed class IceProtocolConnection : IProtocolConnection throw new InvalidDataException( $"Expected '{nameof(IceFrameType.ValidateConnection)}' frame but received frame type '{validateConnectionFrame.FrameType}'."); } + + // The client connection is now connected, so we schedule the first heartbeat. + if (_duplexConnection is IceDuplexConnectionDecorator decorator) + { + decorator.ScheduleHeartbeat(); + } } } catch (OperationCanceledException) diff --git a/tests/IceRpc.Tests/IceProtocolConnectionTests.cs b/tests/IceRpc.Tests/IceProtocolConnectionTests.cs index 0791ccd5a..4232a4102 100644 --- a/tests/IceRpc.Tests/IceProtocolConnectionTests.cs +++ b/tests/IceRpc.Tests/IceProtocolConnectionTests.cs @@ -549,6 +549,43 @@ public async Task Idle_timeout_mismatch_aborts_connection() Throws.InstanceOf().With.Property("IceRpcError").EqualTo(IceRpcError.ConnectionAborted)); } + /// Verifies that a connection where the client does not write anything is not aborted by the server + /// connection idle monitor. + /// This test also verifies that the client idle monitor does not abort the connection when the server + /// does not write anything; it's less interesting since the server always writes a ValidateConnection frame after + /// accepting the connection from the client. + [Test] + public async Task Server_idle_monitor_does_not_abort_connection_when_client_does_not_write_anything() + { + var connectionOptions = new ConnectionOptions + { + IceIdleTimeout = TimeSpan.FromMilliseconds(100) + }; + + await using ServiceProvider provider = new ServiceCollection() + .AddProtocolTest( + Protocol.Ice, + dispatcher: null, + connectionOptions, + connectionOptions) + .BuildServiceProvider(validateScopes: true); + + ClientServerProtocolConnection sut = provider.GetRequiredService(); + (Task clientShutdownRequested, Task serverShutdownRequested) = await sut.ConnectAsync(); + + // Act + await Task.Delay(TimeSpan.FromMilliseconds(400)); // plenty of time for the idle monitor to kick in. + + // Assert + Assert.That(serverShutdownRequested.IsCompleted, Is.False); + Assert.That(clientShutdownRequested.IsCompleted, Is.False); + + // Graceful shutdown. + Task clientShutdown = sut.Client.ShutdownAsync(); + Task serverShutdown = sut.Server.ShutdownAsync(); + Assert.That(async () => await Task.WhenAll(clientShutdown, serverShutdown), Throws.Nothing); + } + /// Verifies that canceling an invocation while the request is being written does not interrupt the write. /// The connection remains active and subsequent request are not affected. [Test] From 0c1b153ac9b4c0f3194a098aa25a47f9dddbbdd3 Mon Sep 17 00:00:00 2001 From: Bernard Normier Date: Sat, 16 Nov 2024 11:52:11 -0500 Subject: [PATCH 2/3] Update comment --- src/IceRpc/Internal/IceDuplexConnectionDecorator.cs | 4 ++-- src/IceRpc/Internal/IceProtocolConnection.cs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/IceRpc/Internal/IceDuplexConnectionDecorator.cs b/src/IceRpc/Internal/IceDuplexConnectionDecorator.cs index 2d9eac39a..d0e23bd0b 100644 --- a/src/IceRpc/Internal/IceDuplexConnectionDecorator.cs +++ b/src/IceRpc/Internal/IceDuplexConnectionDecorator.cs @@ -96,8 +96,8 @@ internal IceDuplexConnectionDecorator( _readIdleTimeout = readIdleTimeout; // can be infinite i.e. disabled _writeIdleTimeout = writeIdleTimeout; _writeTimer = new Timer(_ => sendHeartbeat()); - - // We can't schedule a heartbeat right away because the connection is not connected yet. + // We can't schedule the initial heartbeat yet. The heartbeat is an ice protocol frame; we can send it only once + // the connection is connected at the ice protocol level. } /// Schedules the initial heartbeat. Called by a client IceProtocolConnection once it receives the diff --git a/src/IceRpc/Internal/IceProtocolConnection.cs b/src/IceRpc/Internal/IceProtocolConnection.cs index dd276912c..3f2a289e4 100644 --- a/src/IceRpc/Internal/IceProtocolConnection.cs +++ b/src/IceRpc/Internal/IceProtocolConnection.cs @@ -118,7 +118,7 @@ internal sealed class IceProtocolConnection : IProtocolConnection await SendControlFrameAsync(EncodeValidateConnectionFrame, connectCts.Token).ConfigureAwait(false); // The SendControlFrameAsync is a "write" that schedules a heartbeat when the idle timeout is not - // infinite. So no need to call scheduleHeartbeat. + // infinite. So no need to call ScheduleHeartbeat. } else { From 43d70792c2067af38d1a82a0675bcbba1498e6cd Mon Sep 17 00:00:00 2001 From: Bernard Normier Date: Sat, 16 Nov 2024 11:53:24 -0500 Subject: [PATCH 3/3] More comment update --- src/IceRpc/Internal/IceDuplexConnectionDecorator.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/IceRpc/Internal/IceDuplexConnectionDecorator.cs b/src/IceRpc/Internal/IceDuplexConnectionDecorator.cs index d0e23bd0b..5bda6429c 100644 --- a/src/IceRpc/Internal/IceDuplexConnectionDecorator.cs +++ b/src/IceRpc/Internal/IceDuplexConnectionDecorator.cs @@ -100,7 +100,7 @@ internal IceDuplexConnectionDecorator( // the connection is connected at the ice protocol level. } - /// Schedules the initial heartbeat. Called by a client IceProtocolConnection once it receives the + /// Schedules the initial heartbeat. Called by a client IceProtocolConnection after it receives the /// initial ValidateConnection frame from the server. internal void ScheduleHeartbeat() => RescheduleWriteTimer();