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: incorrect docs on KeepAlive field of Dialer #29089

Open
mikioh opened this Issue Dec 4, 2018 · 4 comments

Comments

Projects
None yet
3 participants
@mikioh
Copy link
Contributor

mikioh commented Dec 4, 2018

What version of Go are you using (go version)?

tip

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

doesn't matter

What did you do?

Run "go doc net Dialer" and took a look at the output

What did you expect to see?

At least the docs introduced by #23459 never uses the phrase "keep-alive period" because the existing implementation tries to tweak the keepalive idle and probe intervals, not the entire keepalive period.

What did you see instead?`

// KeepAlive specifies the keep-alive period for an active
// network connection.
// If zero, keep-alives are enabled if supported by the protocol
// and operating system. Network protocols or operating systems
// that do not support keep-alives ignore this field.
// If negative, keep-alives are disabled.
KeepAlive time.Duration

FWIW, SetKeepAlivePeriod on TCPConn says:

// SetKeepAlivePeriod sets period between keep alives.
func (c *TCPConn) SetKeepAlivePeriod(d time.Duration) error {

@mikioh mikioh added the help wanted label Dec 4, 2018

@mikioh mikioh added this to the Go1.12 milestone Dec 4, 2018

@mikioh

This comment has been minimized.

Copy link
Contributor Author

mikioh commented Dec 4, 2018

In addition, I personally prefer implementing KeepAlive as an entire keepalive period rather than idle and probe intervals like CL 107196.

@bradfitz bradfitz self-assigned this Dec 6, 2018

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Dec 14, 2018

@mikioh, I thought you were saying that 5bd7e9c introduced the wording "keep-alive period" but it does not. It just adds some more words around the existing Dialer.KeepAlive docs.

I'm going to punt this to Go 1.13.

@bradfitz bradfitz modified the milestones: Go1.12, Go1.13 Dec 14, 2018

@bradfitz bradfitz removed their assignment Dec 14, 2018

@bradfitz bradfitz added the NeedsFix label Dec 14, 2018

@mikioh

This comment has been minimized.

Copy link
Contributor Author

mikioh commented Dec 14, 2018

@bradfitz,

Looks like you introduced this confusion by fdfbb40; I should not miss reviewing the CL. People using KeepAlive of Dialer with a positive value assume that it represents an entire TCP keepalive period but it's actually a keepalive interval; the duration between keepalive probes. It's regrettable.

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Dec 30, 2018

Change https://golang.org/cl/155960 mentions this issue: net: correct docs of KeepAlive field in Dialer type

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.