setkeepalive < 1000 silently ignored

[rotu]

Version

22.14.0

Platform

N/A

Subsystem

No response

What steps will reproduce the bug?

Create a TCP socket and call socket.setKeepAlive(true, 400).

How often does it reproduce? Is there a required condition?

Any keepalive less than 1 second.

What is the expected behavior? Why is that the expected behavior?

This should send keepalive packets every 400 ms.

The documentation says as much:

keepAliveInitialDelay {number} If set to a positive number, it sets the
initial delay before the first keepalive probe is sent on an idle socket.
Default: 0.

What do you see instead?

The code truncates to the nearest 1000ms, so the socket doesn't send keepalive packets and no error is issued.

Additional information

It seems this parameter is even broken in the tests.

node/test/parallel/test-net-keepalive.js

Line 42 in 3db5491

connection.setKeepAlive(true, common.platformTimeout(50));

node/test/parallel/test-net-persistent-keepalive.js

Line 30 in 3db5491

const s = clientConnection.setKeepAlive(true, 400);

Ideally, the keepalive interval would be respected in milliseconds. Assuming that's infeasible,

1. a runtime warning should be issued if setKeepAlive is given a value <1000.
2. the documentation should be updated to reflect that, even though the value is given in milliseconds, it will only be respected to the second.

[Emill]

Linux has the requirement that the initial delay must be an integer number of seconds >= 1 and <= 32767, otherwise EINVAL is returned from the kernel. I suggest the millisecond parameter is rounded up to the nearest second. If the number is instead rounded down, the probe is sent earlier than the user instructed. However, it should be capped at 32767. Now an invalid value is silently ignored.

[Emill]

Also, the current optional parameter handling doesn't seem to match the documentation. Current code:

node/lib/net.js

Lines 626 to 628 in 538186b

	Socket.prototype.setKeepAlive = function(enable, initialDelayMsecs) {
	enable = Boolean(enable);
	const initialDelay = ~~(initialDelayMsecs / 1000);

The documentation says socket.setKeepAlive([enable][, initialDelay]).

So, e.g. updating to 3 seconds by calling socket.setKeepAlive(3000) doesn't work.

[yoshigev]

We also hit this problem. I think the documentation should be fixed to mention this.

[efekrskl]

As far as I see libuv uses seconds for this value and any value below 1 returns an error code, so I think the current behaviour makes sense. However, I agree the silent truncation can be confusing. I opened a PR to improve the docs