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

net: use the same default TCPKeepAlive interval in net.Listen and net.Dial #31510

Closed
CAFxX opened this issue Apr 17, 2019 · 7 comments
Closed

net: use the same default TCPKeepAlive interval in net.Listen and net.Dial #31510

CAFxX opened this issue Apr 17, 2019 · 7 comments

Comments

@CAFxX
Copy link
Contributor

@CAFxX CAFxX commented Apr 17, 2019

In #23378 (comment) we noted that there is a mismatch between the default TCP keepalive used by net.Listen (3m, previously in http.ListenAndServe(TLS)) and the one used by net.Dial (15s).

For uniformity, it would be advisable to ensure these two defaults match.

If there are no counter-proposals, my suggestion would be to use 15s as the interval (see #23459 for prior discussion).

An explicit non-goal of this issue is that of making the default value part of the API. For the reasons discussed in https://go-review.googlesource.com/c/go/+/107196 this should remain undefined, and users that have specific requirements in terms of exact keepalive durations should specify a custom configuration.

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Apr 17, 2019

Does http.ListenAndServe still actually set a TCP Keep-Alive? When I looked yesterday, I couldn't find it anymore, and assumed it was now relying on the new net.Listen default.

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Apr 17, 2019

Yeah, this is not in net/http as of 1abf3aa.

The Accept() default is 3 minutes though, not 15s.

@CAFxX

This comment has been minimized.

Copy link
Contributor Author

@CAFxX CAFxX commented Apr 17, 2019

Yeah, but the net.Listen default is 3m because 1abf3aa lifted the default out of ListenAndServe and into net.Listen. This is orthogonal though to the point that these two defaults (in net.Listen and net.Dial) should be the same for consistency.

@FiloSottile FiloSottile changed the title net/http: use the same default TCPKeepAlive interval as net.Dialer net: use the same default TCPKeepAlive interval in net.Listen and net.Dial Apr 17, 2019
@FiloSottile FiloSottile added this to the Go1.13 milestone Apr 17, 2019
@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Apr 17, 2019

Changed the title accordingly. release-blocker because the net.Listen keep-alive is new, so we should decide if we are changing it before shipping it, ideally.

@CAFxX

This comment has been minimized.

Copy link
Contributor Author

@CAFxX CAFxX commented Apr 30, 2019

Are there specific concerns against the approach outlined above?

If there are no counter-proposals, my suggestion would be to use 15s as the interval (see #23459 for prior discussion).

If there are none, I'll go ahead and send a CL.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented May 1, 2019

I can't see why these should be different. Go ahead and send a CL.

@rsc rsc added the NeedsFix label May 1, 2019
@gopherbot gopherbot removed the NeedsDecision label May 1, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented May 5, 2019

Change https://golang.org/cl/175259 mentions this issue: net: use the same default TCPKeepAlive interval for accept

@gopherbot gopherbot closed this in b98cecf May 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.