net: set tcp keepalive delay only if > 0 #3488

cc-int3 · 2022-02-16T23:05:43Z

If tcpkeepalive delay is zero, don't set the TCP_KEEPALIVE sock option.
This will result in a) using the windows system settings or b) if
previously set, no change.

If tcpkeepalive delay is zero, don't set the TCP_KEEPALIVE sock option. This will result in a) using the windows system settings or b) if previously set, no change.

cc-int3 · 2022-02-16T23:05:57Z

for issue #3487

vtjnash · 2022-02-17T16:54:37Z

Can you add a simple test (showing that it returns successfully) and change the unix code to match? The discussion in #3487 is ongoing still, but that would push this towards a state where it can be reviewed in tandem.

cc-int3 · 2022-02-17T20:43:56Z

Sure, it'll take a day or so

Ensure that uv_tcp_keepalive will accept a delay value of 0 ( meaning use previous set value, or system default, if not previously set )

cc-int3 · 2022-02-17T21:55:07Z

added additional tests to existing file.
Linux behavior and windows behavior match (with respect to delay)

saghul

A tiny nit, LGTM otherwise.

saghul · 2022-02-25T23:30:54Z

docs/src/tcp.rst

-    ignored when `enable` is zero.
+    ignored when `enable` is zero. 
+    
+    A `delay` of 0 will imply using the system default setting for tcp keepalives,


Suggested change

A `delay` of 0 will imply using the system default setting for tcp keepalives,

A `delay` of 0 will imply using the system default setting for TCP keepalives,

saghul · 2022-03-03T09:17:23Z

I think we need this logic for (at least) Linux too: https://github.com/torvalds/linux/blob/f1baf68e1383f6ed93eb9cff2866d46562607a43/net/ipv4/tcp.c#L3313

If you call uv_tcp_keepalive(handle, 1, 0) on a connected socket you get UV_EINVAL on Linux, not so on macOS.

I'd suggest we also apply the delay only if it's > 0 for Unix:

libuv/src/unix/tcp.c

Line 383 in 83efa3d

int uv__tcp_keepalive(int fd, int on, unsigned int delay) {

Or maybe we should ignore EINVAL? I guess a platform limitation is that you cannot set the initial delay to something and then want to reset it to the platform default.

Last, the current flags test is insufficient. Because it's applied over an unbound socket, the delay is ignored:

libuv/src/unix/tcp.c

Line 445 in 83efa3d

/* TODO Store delay if uv__stream_fd(handle) == -1 but don't want to enlarge

I think we should at least bind it and that will make the above example fail on Linux.

@bnoordhuis WDYT?

test: call bind to get valid socket for tests

cc-int3 · 2022-03-05T17:04:57Z

Added requested changes.
I'm torn on not setting TCP_KEEPINTVL and TCP_KEEPCNT to the windows ( and documentation ) values in unix when delay == 0 ( I painted myself into a corner with my 'use system defaults' stance ) I really have no strong feelings about those two settings.

saghul · 2022-03-06T07:33:29Z

Looking good! Do we know what other platforms do when trying to set the delay to 0? We now know Windows resets it and Linux errors out.

cc-int3 · 2022-03-12T14:02:05Z

I don't have access to any other platforms, so I'm not sure how to answer. I thought I saw an issue with keepalives on an IBM OS during my original search, but I can't seems to find it now. Sorry I can't be of more help.

stale · 2022-04-16T03:16:52Z

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

saghul · 2022-04-16T06:00:06Z

I'll try to look at what the FreeBSD kernel does shortly.

net: set tcp keepalive delay only if > 0

7158557

If tcpkeepalive delay is zero, don't set the TCP_KEEPALIVE sock option. This will result in a) using the windows system settings or b) if previously set, no change.

test: add tests for tcpkeepalive delay values of 0

68e9592

Ensure that uv_tcp_keepalive will accept a delay value of 0 ( meaning use previous set value, or system default, if not previously set )

Added doc to clarify expected behavior

da66ad2

saghul reviewed Feb 25, 2022

View changes

Fix TCP case

8b17f60

net: only change unix keepalive values if delay>0

b7a54d4

test: call bind to get valid socket for tests

stale bot added the stale label Apr 16, 2022

stale bot removed the stale label Apr 16, 2022

Mar	APR	May
	22
2021	2022	2023

libuv / libuv Public

net: set tcp keepalive delay only if > 0 #3488

net: set tcp keepalive delay only if > 0 #3488

cc-int3 commented Feb 16, 2022

cc-int3 commented Feb 16, 2022

vtjnash commented Feb 17, 2022

cc-int3 commented Feb 17, 2022

cc-int3 commented Feb 17, 2022

saghul left a comment

saghul Feb 25, 2022

cc-int3 Feb 27, 2022

saghul commented Mar 3, 2022

cc-int3 commented Mar 5, 2022

saghul commented Mar 6, 2022

cc-int3 commented Mar 12, 2022

stale bot commented Apr 16, 2022

saghul commented Apr 16, 2022

	A `delay` of 0 will imply using the system default setting for tcp keepalives,
	A `delay` of 0 will imply using the system default setting for TCP keepalives,

libuv / libuv Public

net: set tcp keepalive delay only if > 0 #3488

Are you sure you want to change the base?

net: set tcp keepalive delay only if > 0 #3488

Conversation

cc-int3 commented Feb 16, 2022

cc-int3 commented Feb 16, 2022

vtjnash commented Feb 17, 2022

cc-int3 commented Feb 17, 2022

cc-int3 commented Feb 17, 2022

saghul left a comment

saghul Feb 25, 2022

Choose a reason for hiding this comment

cc-int3 Feb 27, 2022

Choose a reason for hiding this comment

saghul commented Mar 3, 2022

cc-int3 commented Mar 5, 2022

saghul commented Mar 6, 2022

cc-int3 commented Mar 12, 2022

stale bot commented Apr 16, 2022

saghul commented Apr 16, 2022