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.

Sign up for GitHub

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: support TCP_KEEPIDLE, TCP_KEEPINTVL and TCP_KEEPCNT on newer Windows #65817

Closed
panjf2000 opened this issue Feb 20, 2024 · 9 comments
Closed

net: support TCP_KEEPIDLE, TCP_KEEPINTVL and TCP_KEEPCNT on newer Windows #65817

panjf2000 opened this issue Feb 20, 2024 · 9 comments
Assignees
@panjf2000
Labels
NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Milestone
Go1.23

Comments

@panjf2000
Copy link
Member

panjf2000 commented Feb 20, 2024 •
edited

In CL 542275, the implementation on Windows has a few limitations:

  • TCP_KEEPCNT is not supported.
  • TCP_KEEPIDLE and TCP_KEEPINTVL cannot be set separately, they must be set together.

go/src/net/tcpsock.go

Lines 121 to 127 in ff4e45f

// Note that Windows doesn't support setting the KeepAliveIdle and KeepAliveInterval separately.
// It's recommended to set both Idle and Interval to non-negative values on Windows if you
// intend to customize the TCP keep-alive settings.
// By contrast, if only one of Idle and Interval is set to a non-negative value, the other will
// be set to the system default value, and ultimately, set both Idle and Interval to negative
// values if you want to leave them unchanged.
type KeepAliveConfig struct {

These limitations come from SIO_KEEPALIVE_VALS that is used for TCP keep-alive on Windows in Go currently. It's a relatively old Winsock API for setting up the TCP keep-alive mechanism. I've done some more in-depth research on Windows and realized that we can do more things on some relatively newer versions of Windows using setsockopt and getsockopt.

TCP_KEEPIDLE and TCP_KEEPINTVL are available starting with Windows 10, version 1709. TCP_KEEPCNT is available starting with Windows 10, version 1703. Therefore, we can reconcile the implementation on Windows 10, version 1709 and later versions with the implementations on UNIX's.

Follows up #65809

References
@panjf2000 panjf2000 added OS-Windows NeedsFix The path to resolution is known, but the work has not been done. labels Feb 20, 2024
@panjf2000 panjf2000 added this to the Go1.23 milestone Feb 20, 2024
@gopherbot
Copy link

Change https://go.dev/cl/565495 mentions this issue: net: support TCP_KEEPIDLE, TCP_KEEPINTVL and TCP_KEEPCNT on newer Windows

@panjf2000
Copy link
Member Author

panjf2000 commented Feb 20, 2024 •
edited

Looks like we will have to add some new constants, so this issue might need to be bumped up to a proposal.

@panjf2000 panjf2000 changed the title net: improve the TCP keep-alive mechanism on Windows proposal: net: improve the TCP keep-alive mechanism on Windows Feb 20, 2024
@panjf2000 panjf2000 changed the title proposal: net: improve the TCP keep-alive mechanism on Windows proposal: net: support TCP_KEEPIDLE, TCP_KEEPINTVL and TCP_KEEPCNT on newer Windows Feb 20, 2024
@ianlancetaylor
Copy link
Contributor

Can we put the new constants in internal/syscall/windows?

@panjf2000
Copy link
Member Author

Can we put the new constants in internal/syscall/windows?

If that is allowed, then it would make our work on this much easier, because we don't have to go through the proposal process.

@ianlancetaylor
Copy link
Contributor

We can add anything we like to internal/syscall.

@qmuntal

This comment was marked as duplicate.

Sign in to view
@panjf2000 panjf2000 changed the title proposal: net: support TCP_KEEPIDLE, TCP_KEEPINTVL and TCP_KEEPCNT on newer Windows net: support TCP_KEEPIDLE, TCP_KEEPINTVL and TCP_KEEPCNT on newer Windows Feb 21, 2024
@panjf2000
Copy link
Member Author

We've come to a consensus that this feature can be done without exposing any new constants, so taking this issue out of the Proposal process.

@panjf2000 panjf2000 self-assigned this Feb 21, 2024
@dmitshur dmitshur removed the Proposal label Feb 26, 2024
@gopherbot
Copy link

Change https://go.dev/cl/570077 mentions this issue: net,internal/syscall/windows: prove that keep alive options exists

gopherbot pushed a commit that referenced this issue Mar 21, 2024
@qmuntal
net,internal/syscall/windows: prove that keep alive options exists
68a508c 
The net package currently uses windows.SupportFullTCPKeepAlive to
know if TCP_KEEPIDLE, TCP_KEEPINTVL, and TCP_KEEPCNT are available.
This function is a wrapper over the undocumented RtlGetNtVersionNumbers
API, which tests if the Windows version is at least 10.0.16299. This
approach artificially limits the use of TCP_KEEPCNT, which is
available since Windows 10.0.15063. It also uses an undocumented API,
which is not something we want to rely on.

This CL removes windows.SupportFullTCPKeepAlive in favor of dedicated
proves for each option which are not based on the Windows version.

While here, remove some assertions in setKeepAliveCount. It is better
to let the system decide if the value is valid or not.

Updates #65817.

Cq-Include-Trybots: luci.golang.try:gotip-windows-arm64
Change-Id: I0fe70d46c8675eab06c0e4628cf68571b6e50b80
Reviewed-on: https://go-review.googlesource.com/c/go/+/570077
Reviewed-by: Than McIntosh <thanm@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
@gopherbot
Copy link

Change https://go.dev/cl/576435 mentions this issue: net: update the doc for TCPConn.SetKeepAlivePeriod on Windows

gopherbot pushed a commit that referenced this issue Apr 4, 2024
@panjf2000 @gopherbot
net: update the doc for TCPConn.SetKeepAlivePeriod on Windows
99b65ae 
The method comment of TCPConn.SetKeepAlivePeriod had become
obsolete and inaccurate since CL 565495 and CL 570077 were merged.

For #65817

Change-Id: Ide99b2949676d452a505ba6fd634088f05c9df44
Reviewed-on: https://go-review.googlesource.com/c/go/+/576435
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@panjf2000 panjf2000

Labels
NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Projects
None yet
Milestone
Go1.23
Development

No branches or pull requests
5 participants
@dmitshur @ianlancetaylor @qmuntal @panjf2000 @gopherbot