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

set keepalive to appopiate value (since go 1.13 default is 15s) #1992

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

h0nIg
Copy link

@h0nIg h0nIg commented Apr 18, 2024

set keepalive to appropiate value (since go 1.13 default is 15s).

5 minute for initial keepalive checks are way to high, we should orient us here according to go defaults

https://www.reddit.com/r/golang/comments/d7v7dn/psa_go_113_introduces_15_sec_server_tcp/

@h0nIg h0nIg changed the title remove keepalive (since go 1.13 default is 15s) set keepalive to appopiate value (since go 1.13 default is 15s) Apr 18, 2024
@h0nIg
Copy link
Author

h0nIg commented Apr 23, 2024

@paudley "paudley reacted with thumbs down emoji" why?

@paudley
Copy link
Contributor

paudley commented Apr 23, 2024

@paudley "paudley reacted with thumbs down emoji" why?

Sorry, was a mistake! (undone) I'm doing some testing on the effects of shorter keepalives in our environments but I don't have results yet.

@jackc
Copy link
Owner

jackc commented May 9, 2024

That keep alive value goes all the way back to b46ee0a in 2014. At the time Go did not have TCP keepalive enabled by default.

If a change is to be made here, I believe the correct change would be to remove the 5 minute value and actually use the Go default rather than hard code it to the current Go default.

@h0nIg
Copy link
Author

h0nIg commented May 10, 2024

@jackc done

@@ -800,7 +800,8 @@ func parsePort(s string) (uint16, error) {
}

func makeDefaultDialer() *net.Dialer {
return &net.Dialer{KeepAlive: 5 * time.Minute}
// rely on GOLANG KeepAlive settings
return &net.Dialer
Copy link
Owner

Choose a reason for hiding this comment

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

That's not valid syntax.

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

3 participants