diff --git a/src/IceRpc/Internal/IceDuplexConnectionDecorator.cs b/src/IceRpc/Internal/IceDuplexConnectionDecorator.cs index d26885565..5bda6429c 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() { @@ -103,10 +96,14 @@ internal IceDuplexConnectionDecorator( _readIdleTimeout = readIdleTimeout; // can be infinite i.e. disabled _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 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 after 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..3f2a289e4 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]