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

Add tcp option to set keep alive value on the tcp layer #561

Merged
merged 2 commits into from Nov 17, 2021
Merged

Add tcp option to set keep alive value on the tcp layer #561

merged 2 commits into from Nov 17, 2021

Conversation

mogaleaf
Copy link
Contributor

Hi,

Our devices have bandwidth constraint and we can't use the default 15s tcp keep-alive from the go sdk library.
I have set up an option to be able to change it as it is impossible to do so with the library.

Let me know if it could be integrated to the library.

Signed-off-by: cecile cecile.thiebaut@gmail.com

Signed-off-by: cecile <cecile.thiebaut@gmail.com>
@MattBrittan
Copy link
Contributor

MattBrittan commented Nov 17, 2021

Thanks for this; I can see why it might be useful. However I'm not completely comfortable with the implementation due to the added complexity (extra config structure TcpOptions etc) and, if we are allowing the user to configure this, then why not other Dialer options (potentially LocalAddr and FallbackDelay could be useful).

I wonder if it would be simpler to add a Dialer net.Dialer in ClientOptions (default to net.Dialer{} and have SetConnectTimeout manipulate that). You could then add a SetDialer that provides a fully custom Dialer. The Dialer could then be passed into openConnection (simplifying that function). This would mean importing net into options.go but as this already imports a couple of net packages I don't see that being an issue.

@mogaleaf
Copy link
Contributor Author

I have implemented the changes using net.Dialer directly into options.
Set up default timeout to 30 * time.Second for backward compatibility.
ConnectTimeout is used for WebSocket too so I need to keep it at options level and dialer level.
Let me know what you think about this option.

Thanks

@MattBrittan
Copy link
Contributor

Thanks very much,
As you say having the options ConnectTimeout and Dialer is not ideal but I think working around that is likely to introduce further confusion (the alternative being to pass dialer.Timeout to openConnection). The way this is implemented now should provide maximum flexibility with minimal code changes. This passes the tests so I'll accept it.

Matt

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.

None yet

2 participants