Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bugs in idle timeout logic (C#) #3156

Merged
merged 2 commits into from
Nov 16, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions csharp/src/Ice/ConnectionI.cs
Original file line number Diff line number Diff line change
@@ -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.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

_idleTimeoutTransceiver?.scheduleHeartbeat();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can _idleTimeoutTransceiver ever be null here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's null when the idle timeout is <=0 (i.e., disabled)

if (options.idleTimeout > TimeSpan.Zero && !endpoint.datagram())

}
}

29 changes: 14 additions & 15 deletions csharp/src/Ice/Internal/IdleTimeoutTransceiverDecorator.cs
Original file line number Diff line number Diff line change
@@ -2,7 +2,6 @@

#nullable enable

using System.Collections;
using System.Diagnostics;
using System.Net.Sockets;

@@ -61,17 +60,8 @@ public void finishRead(Buffer buf)

public ConnectionInfo getInfo() => _decoratee.getInfo();

public int initialize(Buffer readBuffer, Buffer writeBuffer, ref bool hasMoreData)
{
int op = _decoratee.initialize(readBuffer, writeBuffer, ref hasMoreData);

if (op == SocketOperation.None) // connected
{
rescheduleWriteTimer();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

return op;
}
public int initialize(Buffer readBuffer, Buffer writeBuffer, ref bool hasMoreData) =>
_decoratee.initialize(readBuffer, writeBuffer, ref hasMoreData);

public string protocol() => _decoratee.protocol();

@@ -108,7 +98,11 @@ public int write(Buffer buf)
return op;
}

internal IdleTimeoutTransceiverDecorator(Transceiver decoratee, ConnectionI connection, TimeSpan idleTimeout, bool enableIdleCheck)
internal IdleTimeoutTransceiverDecorator(
Transceiver decoratee,
ConnectionI connection,
TimeSpan idleTimeout,
bool enableIdleCheck)
{
Debug.Assert(idleTimeout > TimeSpan.Zero);

@@ -118,9 +112,12 @@ internal IdleTimeoutTransceiverDecorator(Transceiver decoratee, ConnectionI conn
if (enableIdleCheck)
{
_readTimer = new System.Threading.Timer(_ => connection.idleCheck(_idleTimeout));
}
idleCheckEnabled = enableIdleCheck;

// idleCheckEnabled remains false for now. ConnectionI calls enableIdleCheck() when it becomes Active to
// schedule the first idle check and set idleCheckEnabled to true.
// We don't want the idle check to start when a client connection is waiting for the initial
// ValidateConnection message or when a server connection is in its initial StateHolding state.
}
_writeTimer = new System.Threading.Timer(_ => connection.sendHeartbeat());
}

@@ -142,6 +139,8 @@ internal void disableIdleCheck()
}
}

internal void scheduleHeartbeat() => rescheduleWriteTimer();

private void cancelReadTimer() => _readTimer!.Change(Timeout.InfiniteTimeSpan, Timeout.InfiniteTimeSpan);

private void cancelWriteTimer() => _writeTimer.Change(Timeout.InfiniteTimeSpan, Timeout.InfiniteTimeSpan);