-
Notifications
You must be signed in to change notification settings - Fork 14
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
Refactors how Slic keeps connections alive #3670
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. See comments.
src/IceRpc/Transports/Slic/Internal/SlicDuplexConnectionDecorator.cs
Outdated
Show resolved
Hide resolved
src/IceRpc/Transports/Slic/Internal/SlicDuplexConnectionDecorator.cs
Outdated
Show resolved
Hide resolved
src/IceRpc/Transports/Slic/Internal/SlicDuplexConnectionDecorator.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks sound to me!
src/IceRpc/Transports/Slic/Internal/SlicDuplexConnectionDecorator.cs
Outdated
Show resolved
Hide resolved
// Only client connections send ping frames when idle to keep the server connection alive. The server | ||
// sends back a Pong frame in turn to keep alive the client connection. | ||
_enableIdleTimeoutAndKeepAlive(idleTimeout, IsServer ? null : KeepAlive); | ||
_duplexConnection.Enable(idleTimeout); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be better to keep IDuplexConnection
and then do
if (_duplexConnection is SlicDuplexConnectionDecorator duplexConnectionDecoator)
{
duplexConnectionDecoator.Enable();
}
Then we can make SlicDuplexConnectionDecorator
timers non-nullable, and keep a single constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how this would work.
The difficulty here is the idle timeout is negotiated during connection establishment. At construction time, we don't know if the negotiated idle timeout is good to be infinite or some other value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can pass the idleTimeout
I forget to add it there, and that is not the point. The point is to avoid creating this decorator for the Server connections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need the decorator for server connections too. It implements the idle timer.
|
||
void SendReadPing() | ||
{ | ||
// ReadKeepAlive is no-op unless _pendingPongCount == 0 before the Increment below: we send a Ping only if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is ReadKeepAlive there is no such operation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. It's the current local function, SendReadPing.
void SendReadPing() | ||
{ | ||
// ReadKeepAlive is no-op unless _pendingPongCount == 0 before the Increment below: we send a Ping only if | ||
// there is no outstanding Pong. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is outstanding Pong.
correct here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced by pending Pong, like the field name.
This PR refactors how Slic keeps connections alive.
Fixes #3665.
With this new implementation, Slic keeps connection alive by sending Ping frames from client connections (like before).
The algorithm is now:
This PR also refactors the idle timeout decorator and associated tests. They are now 2 separate decorators, one for Ice and one for Slic. This PR didn't change the Ice logic.