Idle timeout fixes in C++/Java/JS (#3159)

zeroc-ice · Nov 18, 2024 · 51ca2d1 · 51ca2d1
1 parent 892f92b
commit 51ca2d1
Show file tree

Hide file tree

Showing 7 changed files with 53 additions and 37 deletions.
diff --git a/cpp/src/Ice/ConnectionI.cpp b/cpp/src/Ice/ConnectionI.cpp
@@ -2684,6 +2684,12 @@ Ice::ConnectionI::validate(SocketOperation operation)
                     "received ValidateConnection message with unexpected size " + to_string(size)};
             }
             traceRecv(_readStream, this, _logger, _traceLevels);
+
+            // Client connection starts sending heartbeats once it has received the ValidateConnection message.
+            if (_idleTimeoutTransceiver)
+            {
+                _idleTimeoutTransceiver->scheduleHeartbeat();
+            }
         }
     }
 

diff --git a/cpp/src/Ice/IdleTimeoutTransceiverDecorator.cpp b/cpp/src/Ice/IdleTimeoutTransceiverDecorator.cpp
@@ -68,15 +68,7 @@ IdleTimeoutTransceiverDecorator::decoratorInit(const ConnectionIPtr& connection,
 SocketOperation
 IdleTimeoutTransceiverDecorator::initialize(Buffer& readBuffer, Buffer& writeBuffer)
 {
-    SocketOperation op = _decoratee->initialize(readBuffer, writeBuffer);
-
-    if (op == SocketOperationNone) // connected
-    {
-        // reschedule because Ice often writes to a client connection before it's connected.
-        _timer->reschedule(_heartbeatTimerTask, chrono::milliseconds(_idleTimeout) / 2);
-    }
-
-    return op;
+    return _decoratee->initialize(readBuffer, writeBuffer);
 }
 
 IdleTimeoutTransceiverDecorator::~IdleTimeoutTransceiverDecorator()
@@ -173,3 +165,11 @@ IdleTimeoutTransceiverDecorator::disableIdleCheck()
         _idleCheckEnabled = false;
     }
 }
+
+void
+IdleTimeoutTransceiverDecorator::scheduleHeartbeat()
+{
+    // Reschedule because the connection establishment may have already written to the connection and scheduled a
+    // heartbeat.
+    _timer->reschedule(_heartbeatTimerTask, chrono::milliseconds(_idleTimeout) / 2);
+}
diff --git a/cpp/src/Ice/IdleTimeoutTransceiverDecorator.h b/cpp/src/Ice/IdleTimeoutTransceiverDecorator.h
@@ -67,6 +67,7 @@ namespace IceInternal
         bool idleCheckEnabled() const noexcept { return _idleCheckEnabled; }
         void enableIdleCheck();
         void disableIdleCheck();
+        void scheduleHeartbeat();
 
     private:
         const TransceiverPtr _decoratee;

diff --git a/java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/ConnectionI.java b/java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/ConnectionI.java
@@ -1740,8 +1740,7 @@ private boolean validate(int operation) {
                                     + messageType
                                     + " over a connection that is not yet validated.");
                 }
-                _readStream.readByte(); // Ignore compression status for
-                // validate connection.
+                _readStream.readByte(); // Ignore compression status for validate connection.
                 int size = _readStream.readInt();
                 if (size != Protocol.headerSize) {
                     throw new MarshalException(
@@ -1750,6 +1749,12 @@ private boolean validate(int operation) {
                                     + ".");
                 }
                 TraceUtil.traceRecv(_readStream, this, _logger, _traceLevels);
+
+                // Client connection starts sending heartbeats once it has received the
+                // ValidateConnection message.
+                if (_idleTimeoutTransceiver != null) {
+                    _idleTimeoutTransceiver.scheduleHeartbeat();
+                }
             }
         }
 

diff --git a/java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/IdleTimeoutTransceiverDecorator.java b/java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/IdleTimeoutTransceiverDecorator.java
@@ -9,7 +9,7 @@
 // Decorates Transceiver to send heartbeats and optionally detect when no byte is received/read for
 // a while.
 // This decorator must not be applied on UDP connections.
-class IdleTimeoutTransceiverDecorator implements Transceiver {
+final class IdleTimeoutTransceiverDecorator implements Transceiver {
     private final Transceiver _decoratee;
     private final int _idleTimeout;
     private final ScheduledExecutorService _scheduledExecutorService;
@@ -36,13 +36,7 @@ public void setReadyCallback(ReadyCallback callback) {
 
     @Override
     public int initialize(Buffer readBuffer, Buffer writeBuffer) {
-        int op = _decoratee.initialize(readBuffer, writeBuffer);
-
-        if (op == SocketOperation.None) { // connected
-            rescheduleWriteTimer();
-        }
-
-        return op;
+        return _decoratee.initialize(readBuffer, writeBuffer);
     }
 
     @Override
@@ -112,7 +106,7 @@ public void setBufferSize(int rcvSize, int sndSize) {
         _decoratee.setBufferSize(rcvSize, sndSize);
     }
 
-    public IdleTimeoutTransceiverDecorator(
+    IdleTimeoutTransceiverDecorator(
             Transceiver decoratee,
             ConnectionI connection,
             int idleTimeout,
@@ -126,24 +120,30 @@ public IdleTimeoutTransceiverDecorator(
         _sendHeartbeat = () -> connection.sendHeartbeat();
     }
 
-    public boolean isIdleCheckEnabled() {
+    boolean isIdleCheckEnabled() {
         return _idleCheckEnabled;
     }
 
-    public void enableIdleCheck() {
+    void enableIdleCheck() {
         if (!_idleCheckEnabled && _idleCheck != null) {
             rescheduleReadTimer();
             _idleCheckEnabled = true;
         }
     }
 
-    public void disableIdleCheck() {
+    void disableIdleCheck() {
         if (_idleCheckEnabled && _idleCheck != null) {
             cancelReadTimer();
             _idleCheckEnabled = false;
         }
     }
 
+    void scheduleHeartbeat() {
+        // Reschedule because the connection establishment may have already written to the
+        // connection and scheduled a heartbeat.
+        rescheduleWriteTimer();
+    }
+
     private void cancelReadTimer() {
         if (_readTimerFuture != null) {
             _readTimerFuture.cancel(false);

diff --git a/js/src/Ice/ConnectionI.js b/js/src/Ice/ConnectionI.js
@@ -1101,6 +1101,10 @@ export class ConnectionI {
             this._logger.trace(traceLevels.networkCat, `established ${this._endpoint.protocol()} connection\n${this}`);
         }
 
+        if (this._transceiver instanceof IdleTimeoutTransceiverDecorator) {
+            // Client connection starts sending heartbeats once it has received the ValidateConnection message.
+            this._transceiver.scheduleHeartbeat();
+        }
         return true;
     }
 

diff --git a/js/src/Ice/IdleTimeoutTransceiverDecorator.js b/js/src/Ice/IdleTimeoutTransceiverDecorator.js
@@ -1,8 +1,4 @@
-//
-// Copyright (c) ZeroC, Inc. All rights reserved.
-//
-
-import { SocketOperation } from "./SocketOperation.js";
+// Copyright (c) ZeroC, Inc.
 
 export class IdleTimeoutTransceiverDecorator {
     constructor(decoratee, connection, timer, idleTimeout, enableIdleCheck) {
@@ -11,22 +7,20 @@ export class IdleTimeoutTransceiverDecorator {
         this._decoratee = decoratee;
         this._idleTimeout = idleTimeout * 1000; // Convert seconds to milliseconds
         this._timer = timer;
-        this._enableIdleCheck = enableIdleCheck;
         this._connection = connection;
+
+        // _idleCheckEnabled is initially enableIdleCheck (by default, true) unlike C++/C#/Java.
+        // Since JS supports only client connections, we know the connection will read at a minimum the initial
+        // ValidateConnection message, and this reading will start or reset the read timer.
+        this._idleCheckEnabled = enableIdleCheck;
     }
 
     setCallbacks(connectedCallback, bytesAvailableCallback, bytesWrittenCallback) {
         this._decoratee.setCallbacks(connectedCallback, bytesAvailableCallback, bytesWrittenCallback);
     }
 
     initialize(readBuffer, writeBuffer) {
-        const op = this._decoratee.initialize(readBuffer, writeBuffer);
-        if (op == SocketOperation.None) {
-            // connected
-            this.rescheduleReadTimer();
-            this.rescheduleWriteTimer();
-        }
-        return op;
+        return this._decoratee.initialize(readBuffer, writeBuffer);
     }
 
     register() {
@@ -79,6 +73,12 @@ export class IdleTimeoutTransceiverDecorator {
         return this._decoratee.toString();
     }
 
+    scheduleHeartbeat() {
+        // Reschedule because the connection establishment may have already written to the connection and scheduled a
+        // heartbeat.
+        this.rescheduleWriteTimer();
+    }
+
     cancelReadTimer() {
         if (this._readTimerToken !== undefined) {
             this._timer.cancel(this._readTimerToken);
@@ -94,7 +94,7 @@ export class IdleTimeoutTransceiverDecorator {
     }
 
     rescheduleReadTimer() {
-        if (this._enableIdleCheck) {
+        if (this._idleCheckEnabled) {
             this.cancelReadTimer();
             this._readTimerToken = this._timer.schedule(() => {
                 this._connection.idleCheck(this._idleTimeout);