Fix bugs in idle timeout logic (C#) #3156
Conversation
|
|
||
| if (op == SocketOperation.None) // connected | ||
| { | ||
| rescheduleWriteTimer(); |
There was a problem hiding this comment.
Bug 1:
Scheduling the first "send heartbeat" in initialize is too early for a client connection that waits for a while to get the ValidateConnection message from the server. We can end up in the situation where the client connection sends its first heartbeat while the Connection is not Active yet, and all the send heartbeats are then lost if the client doesn't write anything else.
| @@ -126,7 +117,10 @@ internal IdleTimeoutTransceiverDecorator(Transceiver decoratee, ConnectionI conn | |||
|
|
|||
| internal void enableIdleCheck() | |||
| { | |||
| if (!idleCheckEnabled && _readTimer is not null) | |||
| // idleCheckEnabled is already true when enableIdleCheck() is called on a TCP server connection that becomes | |||
There was a problem hiding this comment.
Bug 2:
we can't count on a connection to read anything to schedule the very first idle check.
For example, a TCP server connection can become active and not read anything ever. This should trigger an idle timeout.
There was a problem hiding this comment.
I don't understand this second fix.
There was a problem hiding this comment.
Take a TCP server connection, where the client doesn't send any request or ValidateConnection frame. Maybe because it's not an Ice client.
In this case, when do we schedule the very fist idle check? Can't be in a read since we don't read anything. A good spot is when the ConnectionI becomes active for the first time and calls enableIdleCheck.
There was a problem hiding this comment.
but why is idleCheckEnabled already true?
There was a problem hiding this comment.
It's set to true by the IdleTimeoutTransceiverDecorator constructor.
Maybe a better fix would be to set it to false in the constructor, and rely on the first enableIdleCheck() (from the first activate) to flip it to true?
There was a problem hiding this comment.
Fixed as suggested: the constructor no longer sets idleCheckEnabled to true.
| @@ -1941,6 +1941,9 @@ private bool validate(int operation) | |||
| throw new MarshalException($"Received ValidateConnection message with unexpected size {size}."); | |||
| } | |||
| TraceUtil.traceRecv(_readStream, this, _logger, _traceLevels); | |||
|
|
|||
| // Client connection starts sending heartbeats once it's received the ValidateConnection message. | |||
| _idleTimeoutTransceiver?.scheduleHeartbeat(); | |||
There was a problem hiding this comment.
Can _idleTimeoutTransceiver ever be null here?
There was a problem hiding this comment.
Yes, it's null when the idle timeout is <=0 (i.e., disabled)
ice/csharp/src/Ice/ConnectionI.cs
Line 1410 in 54548fe
| @@ -1941,6 +1941,9 @@ private bool validate(int operation) | |||
| throw new MarshalException($"Received ValidateConnection message with unexpected size {size}."); | |||
| } | |||
| TraceUtil.traceRecv(_readStream, this, _logger, _traceLevels); | |||
|
|
|||
| // Client connection starts sending heartbeats once it's received the ValidateConnection message. | |||
There was a problem hiding this comment.
even if it doesn't write anything (a rare situation).
On the server side, the writing of the initial ValidateConnection message schedules the first heartbeat.
This PR fixes two small bugs in the idle timeout logic (in C# - will port to other languages in a follow-up PR).