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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9a761f9
Fix bugs in idle timeout logic (C#)
bernardnormier Nov 15, 2024
c060474
More fixes
bernardnormier Nov 15, 2024
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

ice/csharp/src/Ice/ConnectionI.cs

Line 1410 in 54548fe

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

}
}

Expand Down
20 changes: 8 additions & 12 deletions csharp/src/Ice/Internal/IdleTimeoutTransceiverDecorator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,17 +61,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();

Expand Down Expand Up @@ -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
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 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.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this second fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

but why is idleCheckEnabled already true?

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed as suggested: the constructor no longer sets idleCheckEnabled to true.

// Active for the first time.

if (_readTimer is not null)
{
rescheduleReadTimer();
idleCheckEnabled = true;
Expand All @@ -142,6 +136,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);
Expand Down
Loading