Skip to content

client: Tweak the keepalive interval and timeout #548

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

Merged
cdecker merged 1 commit into from
Dec 3, 2024
Merged

client: Tweak the keepalive interval and timeout #548

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions libs/gl-client/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ pub use lightning_signer::bitcoin;
pub use lightning_signer::lightning;
pub use lightning_signer::lightning_invoice;

pub(crate) const TCP_KEEPALIVE: Duration = Duration::from_secs(5);
pub(crate) const TCP_KEEPALIVE_TIMEOUT: Duration = Duration::from_secs(90);
pub(crate) const TCP_KEEPALIVE: Duration = Duration::from_secs(1);
pub(crate) const TCP_KEEPALIVE_TIMEOUT: Duration = Duration::from_secs(5);
Copy link
Contributor

@roeierez roeierez Nov 29, 2024

Choose a reason for hiding this comment

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

I think it makes sense to set the keep alive timeout not more than the interval otherwise we may end up sending a second keepalive packet before the first one is ack'd.
Any reason you see not to set it for 1 second?

Copy link
Collaborator Author

@cdecker cdecker Nov 30, 2024

Choose a reason for hiding this comment

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

It's a balance of eager timeout vs keeping connections more stable. The timeout is counted for each packet, while the interval tells us how to send the ping, the timeout tells us how long to wait for pings. Here we pipeline up to 5 pings before we require the first pong to be back. So technically, if we send 2 pings, the first one gets replied to, the second one doesn't, then at second 6 we reconnect. If we set the interval and timeout to the same, we'd wait for 10 seconds, because we send only every 5s, and wait for a reply for 5s.

Pipelining pings can help reduce the time to recovery, without increasing the sensitivity to latency. Timeout on the other hand controls latency sensitivity, and as mentioned, I'm hoping that for interruptions <5s the TCP connection can just resume, delivering a late pong but keeping it inside of the timeout. Overall I think this will minimize the number of failed grpc calls.

Copy link
Collaborator Author

@cdecker cdecker Nov 30, 2024

Choose a reason for hiding this comment

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

I guess long story short, it's a parameter that we should likely tweak as we learn more about the network constraints we work with.

Copy link
Contributor

@roeierez roeierez Nov 30, 2024

Choose a reason for hiding this comment

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

If we set the interval and timeout to the same, we'd wait for 10 seconds, because we send only every 5s, and wait for a reply for 5s.

I actually commented about making the timeout the same as the interval (both 1 seconds) which means sending every 1 seconds and waiting up to 1 second for ack, lowering the max time to detect network change to 1 second overall and lowering even more the chances to get transport errors (if the call falls into the timeout time frame).
I guess the main question is do we foresee that on a stable connection we will have more than 1 second latency for replying a single tcp packet?
I agree that this fix by itself is a significant improvement.

Copy link
Collaborator Author

@cdecker cdecker Dec 1, 2024

Choose a reason for hiding this comment

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

The best measure we currently have is the RTT that we get from the signer, which admittedly is a rather large request and response so bandwidth also influences this. We see RTTs up to 1.5s relatively regularly, those would be the affected nodes getting disconnected with timeout=1s, if ping messages (unloaded lines) also have a similar RTT.

Copy link
Contributor

@roeierez roeierez Dec 1, 2024

Choose a reason for hiding this comment

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

In my tests tracing the keep alive packets I see response in milliseconds but perhaps my bandwidth is better than the average and we should prepare for higher latency. I am ok to start with 5 seconds and gather some feedback from the field.


pub mod runes;
Loading