Skip to content

add tcp keepalive option to connection settings#440

Open
jbbjarnason wants to merge 1 commit intoisoos:masterfrom
centroid-is:add-keepalive
Open

add tcp keepalive option to connection settings#440
jbbjarnason wants to merge 1 commit intoisoos:masterfrom
centroid-is:add-keepalive

Conversation

@jbbjarnason
Copy link

@jbbjarnason jbbjarnason commented Feb 7, 2026

Disclaimer: This code was generated by claude, but reviewed and tested manually.

@isoos
Copy link
Owner

isoos commented Feb 7, 2026

@jbbjarnason: what do you think about having a reasonable defaults and reduce the settings to e.g. two: the period of keepalives in seconds (if 0 we do not set the keepalive), and how many missed response renders it disconnected? I don't really see where you wouldn't want to set these, so it may be default on?

It would be nice to see a test which is able to detect that the e.g. 2-seconds keepalive after 5 failed attempt is rendering the connection as dead. Would you be up for that?

@jbbjarnason
Copy link
Author

Would you like to reuse the keep alive interval as keep alive Idle, or would you like a constant keep alive idle parameter ?

Yes I can add a test, I only tested this manually on Mac and Linux, would be beneficial to test on all platforms.

@isoos
Copy link
Owner

isoos commented Feb 7, 2026

Would you like to reuse the keep alive interval as keep alive Idle, or would you like a constant keep alive idle parameter ?

I was thinking to reuse the keep alive interval as keep alive idle, but I'm not familiar with best practices here.

Yes I can add a test, I only tested this manually on Mac and Linux, would be beneficial to test on all platforms.

(Github action) CI is running on Linux only (so far), if you want to add a parallel macos platform, happy to take it, but I don't mind keeping it Linux-only (as long as you are happy with it on mac).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants