Use TCP keepalive probes to detect local log stream closing #1220
Conversation
CameronDiver
requested review from
dfunckt,
zvin,
shaunmulligan,
hedss and
pdcastro
package.json
Outdated
| @@ -153,6 +154,7 @@ | |||
| "moment": "^2.24.0", | |||
| "moment-duration-format": "^2.2.1", | |||
| "mz": "^2.6.0", | |||
| "net-keepalive": "^1.2.1", | |||
There was a problem hiding this comment.
From https://www.npmjs.com/package/net-keepalive:
The Missing (TCP_KEEPINTVL and TCP_KEEPCNT) SO_KEEPALIVE socket option setters and getters for Node using ffi module. Tested on linux, should work on osx and freebsd.
I think it is OK for this feature/fix to apply to Linux only (maybe macOS). The question is: does it raise any errors when it is run on Windows? Because raising errors would be a problem. :-)
There was a problem hiding this comment.
So, in Windows 10, this should work as expected as the setsockopt() includes both TCP_KEEPINTVL and TCP_KEEPCNT (which is what the underlying library uses), see here. However, in prior versions, these were set as registry keys and will simply be ignored.
I don't think this'll throw an error, but it's probably worth checking.
There was a problem hiding this comment.
Yeah I initially misread it to support the 3 big ones, testing on a windows vm now.
There was a problem hiding this comment.
This module isn't supported on windows. Will merge this try to find a method of achieving the same thing on Windows.
There was a problem hiding this comment.
@CameronDiver, your laptop runs Linux, right? I offer to test it on macOS, just in case. I've tried it now, but it's failing for other reasons. I'll try again when the build tests are passing.
CameronDiver
force-pushed
the
1219-tcp-keepalive
branch
4 times, most recently
from
5250099
to
96c975d
Compare
Change-type: patch Closes: #1219 Signed-off-by: Cameron Diver <[email protected]>
Change-type: patch Signed-off-by: Cameron Diver <[email protected]>
Since node 12.17.0 setKeepAlive also sets TCP_KEEPCNT=10 (vs 5 that we had) TCP_KEEPINTVL=1 (vs 5s that we had) Change-type: patch See: https://nodejs.org/docs/latest-v14.x/api/net.html#net_socket_setkeepalive_enable_initialdelay See: #1220
Since node 12.17.0 setKeepAlive also sets TCP_KEEPCNT=10 (vs 5 that we had) TCP_KEEPINTVL=1 (vs 5s that we had) Change-type: patch See: https://nodejs.org/docs/latest-v14.x/api/net.html#net_socket_setkeepalive_enable_initialdelay See: #1220
Since node 12.17.0 setKeepAlive also sets TCP_KEEPCNT=10 (vs 5 that we had) TCP_KEEPINTVL=1 (vs 5s that we had) Change-type: patch See: https://nodejs.org/docs/latest-v14.x/api/net.html#net_socket_setkeepalive_enable_initialdelay See: #1220
Since node 12.17.0 setKeepAlive also sets TCP_KEEPCNT=10 (vs 5 that we had) TCP_KEEPINTVL=1 (vs 5s that we had) Change-type: patch See: https://nodejs.org/docs/latest-v14.x/api/net.html#net_socket_setkeepalive_enable_initialdelay See: #1220
Since node 12.17.0 setKeepAlive also sets TCP_KEEPCNT=10 (vs 5 that we had) TCP_KEEPINTVL=1 (vs 5s that we had) Change-type: patch See: https://nodejs.org/docs/latest-v14.x/api/net.html#net_socket_setkeepalive_enable_initialdelay See: #1220
Since node 12.17.0 setKeepAlive also sets TCP_KEEPCNT=10 (vs 5 that we had) TCP_KEEPINTVL=1 (vs 5s that we had) Change-type: patch See: https://nodejs.org/docs/latest-v14.x/api/net.html#net_socket_setkeepalive_enable_initialdelay See: #1220
Since node 12.17.0 setKeepAlive also sets TCP_KEEPCNT=10 (vs 5 that we had) TCP_KEEPINTVL=1 (vs 5s that we had) Change-type: patch See: https://nodejs.org/docs/latest-v14.x/api/net.html#net_socket_setkeepalive_enable_initialdelay See: #1220
Change-type: patch
Closes: #1219
Signed-off-by: Cameron Diver [email protected]
Contributor checklist