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

Jump to bottom

net: DialTCP doesn't set TCP keepalive (unlike Dialer.Dial) #49345

Closed
database64128 opened this issue Nov 4, 2021 · 10 comments
Closed

net: DialTCP doesn't set TCP keepalive (unlike Dialer.Dial) #49345

database64128 opened this issue Nov 4, 2021 · 10 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@database64128
Copy link
Contributor

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

All current versions apply.

Does this issue reproduce with the latest release?

Yes

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

Not platform specific.

What did you do?

Call net.DialTCP().

What did you expect to see?

The default 15s TCP keepalive setting is applied to the connection.

What did you see instead?

TCP keepalive is not enabled.

Merging this section into newTCPConn should solve this disparity between net.Dialer.Dial[Context]() and net.DialTCP().
@seankhliao seankhliao changed the title TCP Keepalive is not enabled by default for connections made by net.DialTCP() net: DialTCP doesn't set TCP keepalive (unlike Dialer.Dial) Nov 4, 2021
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 4, 2021
@seankhliao
Copy link
Member

cc @ianlancetaylor @neild

@neild
Copy link
Contributor

neild commented Nov 4, 2021

DialTCP's documentation states:

DialTCP acts like Dial for TCP networks.

So it seems reasonable that DialTCP should apply the same default configuration as Dial does.

@ianlancetaylor
Copy link
Contributor

There is a key difference, though: Dial permits controlling the keep alive setting via the Dialer.KeepAlive field. There is no way to control the keep alive setting for DialTCP, and no way to change it afterward.

If #49097 is adopted then we can set keep alive for the new DialTCP method of Dialer. When we do that it will be reasonable to change the net.DialTCP function, as there will be a way to change the default. Until then, or until there is some other mechanism to change the keep alive, I don't think we can do this.

@neild
Copy link
Contributor

neild commented Nov 4, 2021

Wouldn't the way to control the keep alive setting be to use Dialer.Dial and type assert the resulting net.Conn to a *TCPConn?

@ianlancetaylor
Copy link
Contributor

That works, but then you aren't using net.DialTCP. Maybe I'm missing something.

@neild
Copy link
Contributor

neild commented Nov 4, 2021

I guess I'm thinking that net.DialTCP is effectively a convenience wrapper for net.Dial("tcp", ...), avoiding the need to type assert the result to a *net.TCPConn. If that's the case, then it seems reasonable for it to use the default value for any net.Dial configuration settings.

Maybe I'm missing something.

@ianlancetaylor
Copy link
Contributor

As I see it the main reason to use net.DialTCP is to pass a *net.TCPAddr for the addresses rather than having to convert them to a string for net.Dial.

@database64128
Copy link
Contributor Author

database64128 commented Nov 5, 2021 •
edited

There is a key difference, though: Dial permits controlling the keep alive setting via the Dialer.KeepAlive field. There is no way to control the keep alive setting for DialTCP, and no way to change it afterward.

You can change it afterwards by calling net.TCPConn.SetKeepAlive and net.TCPConn.SetKeepAlivePeriod right after net.DialTCP.

as there will be a way to change the default.

One could also argue there's never been a way to override the default of setting TCP_NODELAY socket option in these dial functions and methods. I think it's safe to make setting TCP_KEEPALIVE/INTVL/IDLE the default with net.DialTCP, because these socket options, just like TCP_NODELAY, can be changed anytime during the socket's lifetime. It's as easy as simply calling net.TCPConn.SetKeepAlive/net.TCPConn.SetKeepAlivePeriod/net.TCPConn.SetNoDelay.

@ianlancetaylor
Copy link
Contributor

Ah, sorry, missed the SetKeepAlive method. Sure, this seems fine.

@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
database64128 added a commit to database64128/go that referenced this issue Nov 4, 2022
@database64128
net: unify TCP keepalive behavior
0d379fd 
CL 107196 introduced a default TCP keepalive interval for Dialer and
TCPListener (used by both ListenConfig and ListenTCP). Leaving DialTCP
out was likely an oversight.

DialTCP's documentation says it "acts like Dial". Therefore it's natural
to also expect DialTCP to enable TCP keepalive by default.

This commit addresses this disparity by moving the enablement logic down
to the newTCPConn function, which is used by both dialer and listener.

Fixes golang#49345
@database64128 database64128 mentioned this issue Nov 4, 2022
Closed
@gopherbot
Copy link

Change https://go.dev/cl/447917 mentions this issue: net: unify TCP keepalive behavior

database64128 added a commit to database64128/go that referenced this issue Nov 9, 2022
@database64128
net: unify TCP keepalive behavior
801b8dc 
CL 107196 introduced a default TCP keepalive interval for Dialer and
TCPListener (used by both ListenConfig and ListenTCP). Leaving DialTCP
out was likely an oversight.

DialTCP's documentation says it "acts like Dial". Therefore it's natural
to also expect DialTCP to enable TCP keepalive by default.

This commit addresses this disparity by moving the enablement logic down
to the newTCPConn function, which is used by both dialer and listener.

Fixes golang#49345
database64128 added a commit to database64128/go that referenced this issue Nov 10, 2022
@database64128
net: unify TCP keepalive behavior
5c2f1cb 
CL 107196 introduced a default TCP keepalive interval for Dialer and
TCPListener (used by both ListenConfig and ListenTCP). Leaving DialTCP
out was likely an oversight.

DialTCP's documentation says it "acts like Dial". Therefore it's natural
to also expect DialTCP to enable TCP keepalive by default.

This commit addresses this disparity by moving the enablement logic down
to the newTCPConn function, which is used by both dialer and listener.

Fixes golang#49345
@golang golang locked and limited conversation to collaborators Nov 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unplanned
Development

Successfully merging a pull request may close this issue.

net: unify TCP keepalive behavior database64128/go
5 participants
@neild @ianlancetaylor @gopherbot @seankhliao @database64128