Update client IceProtocolConnection to schedule first heartbeat only … (

#4115)

src/IceRpc/Internal/IceDuplexConnectionDecorator.cs

@@ -17,15 +17,8 @@ internal class IceDuplexConnectionDecorator : IDuplexConnection
     private readonly TimeSpan _writeIdleTimeout;
     private readonly Timer _writeTimer;
 
-    public async Task<TransportConnectionInformation> 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<TransportConnectionInformation> 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.
     }
 
+    /// <summary>Schedules the initial heartbeat. Called by a client IceProtocolConnection after it receives the
+    /// initial ValidateConnection frame from the server.</summary>
+    internal void ScheduleHeartbeat() => RescheduleWriteTimer();
+
     /// <summary>Cancels the write timer.</summary>
     private void CancelWriteTimer() => _writeTimer.Change(Timeout.InfiniteTimeSpan, Timeout.InfiniteTimeSpan);
 

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)

tests/IceRpc.Tests/IceIdleTimeoutTests.cs

@@ -1,11 +1,12 @@
 // Copyright (c) ZeroC, Inc.
 
+using IceRpc.Internal;
 using IceRpc.Tests.Common;
 using Microsoft.Extensions.DependencyInjection;
 using NUnit.Framework;
 using System.Buffers;
 
-namespace IceRpc.Internal;
+namespace IceRpc.Tests;
 
 [Parallelizable(scope: ParallelScope.All)]
 public class IceIdleTimeoutTests
@@ -77,4 +78,41 @@ public async Task Ice_send_heartbeat_action_is_called()
             TimeSpan.FromMilliseconds(Environment.TickCount64) - startTime,
             Is.LessThan(TimeSpan.FromMilliseconds(500)));
     }
+
+    /// <summary>Verifies that a connection where the client does not write anything is not aborted by the server
+    /// connection idle monitor.</summary>
+    /// <remarks>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.</remarks>
+    [Test][NonParallelizable]
+    public async Task Server_idle_monitor_does_not_abort_connection_when_client_does_not_write_anything()
+    {
+        var connectionOptions = new ConnectionOptions
+        {
+            IceIdleTimeout = TimeSpan.FromMilliseconds(300)
+        };
+
+        await using ServiceProvider provider = new ServiceCollection()
+            .AddProtocolTest(
+                Protocol.Ice,
+                dispatcher: null,
+                connectionOptions,
+                connectionOptions)
+            .BuildServiceProvider(validateScopes: true);
+
+        ClientServerProtocolConnection sut = provider.GetRequiredService<ClientServerProtocolConnection>();
+        (Task clientShutdownRequested, Task serverShutdownRequested) = await sut.ConnectAsync();
+
+        // Act
+        await Task.Delay(TimeSpan.FromMilliseconds(900)); // 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);
+    }
 }