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: DialTCP doesn't set TCP keepalive (unlike Dialer.Dial) #49345

Open
database64128 opened this issue Nov 4, 2021 · 9 comments
Open

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

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

Comments

@database64128
Copy link
Contributor

database64128 commented Nov 4, 2021

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

seankhliao commented Nov 4, 2021

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

ianlancetaylor commented Nov 4, 2021

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

ianlancetaylor commented Nov 4, 2021

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

ianlancetaylor commented Nov 4, 2021

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

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

ianlancetaylor commented Nov 5, 2021

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

@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants