Separate TcpKeepAliveIdle and TcpKeepAliveInterval check logic #1484
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1484 +/- ##
==========================================
+ Coverage 39.52% 39.56% +0.04%
==========================================
Files 598 595 -3
Lines 35068 34944 -124
==========================================
- Hits 13860 13827 -33
+ Misses 19681 19603 -78
+ Partials 1527 1514 -13
Continue to review full report at Codecov.
|
transport/internet/sockopt_darwin.go
Outdated
| @@ -9,6 +9,8 @@ const ( | |||
| TCP_FASTOPEN_SERVER = 0x01 // nolint: revive,stylecheck | |||
| // TCP_FASTOPEN_CLIENT is the value to enable TCP fast open on darwin for client connections. | |||
| TCP_FASTOPEN_CLIENT = 0x02 // nolint: revive,stylecheck | |||
| // syscall.TCP_KEEPINTVL is missing on some darwin architectures. | |||
| sysTCP_KEEPINTVL = 0x101 // nolint: revive,stylecheck | |||
There was a problem hiding this comment.
Which darwin architectures are missing TCP_KEEPINTVL? We are using x/sys/unix here, not syscall, please double check.
There was a problem hiding this comment.
I've copied this from https://github.com/golang/go/blob/master/src/net/tcpsockopt_darwin.go
There was a problem hiding this comment.
That's not where you should be looking at.
TCP_KEEPINTVL is present for both darwin-amd64 and darwin-arm64. You can safely use unix.TCP_KEEPINTVL.
There was a problem hiding this comment.
Your recommendation have been adapted at 406e0f7.
transport/internet/sockopt_darwin.go
Outdated
| if config.TcpKeepAliveInterval > 0 { | ||
| if err := unix.SetsockoptInt(int(fd), unix.IPPROTO_TCP, sysTCP_KEEPINTVL, int(config.TcpKeepAliveIdle)); err != nil { | ||
| return newError("failed to set TCP_KEEPIDLE", err) | ||
| } |
There was a problem hiding this comment.
I think moving setsockopt(TCP_KEEPALIVE) and setsockopt(TCP_KEEPINTVL) out of the if config.TcpKeepAliveIdle > 0 || config.TcpKeepAliveInterval > 0 block would make it more readable.
There was a problem hiding this comment.
Your recommendation have been adapted at 406e0f7.
|
@xiaokangwang, thanks for the fix. I'll adapt it for #1483 and update shortly. |
This patch allows to separately configure both parameters.