net: warn on keep-alive delays truncated to zero

doc/api/net.md

@@ -1669,7 +1669,11 @@ corresponding system default unchanged.
 
 `initialDelay` and `interval` are specified in milliseconds but the
 underlying socket options are configured in whole seconds; the values are
-divided by `1000` and rounded down before being applied.
+divided by `1000` and rounded down before being applied. Sub-second timings
+cannot be expressed, so a positive value below `1000` throws
+[`ERR_OUT_OF_RANGE`][] rather than leaving the corresponding system default in
+place. The largest delay the socket options can carry is `32767` seconds, and a
+value above that throws as well.
 
 Enabling the keep-alive functionality will set the following socket options:
 
@@ -1688,6 +1692,12 @@ those platforms.
 added:
  - v26.4.0
  - v24.19.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/65528
+    description: A positive `initialDelay` or `interval` that cannot be applied
+                 as requested now throws `ERR_OUT_OF_RANGE` instead of being
+                 silently altered.
 -->
 
 * `options` {Object}
@@ -1709,6 +1719,11 @@ socket.setKeepAlive({ enable: true, initialDelay: 1000, interval: 1000, count: 1
 <!-- YAML
 added: v0.1.92
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/65528
+    description: A positive `initialDelay` or `interval` that cannot be applied
+                 as requested now throws `ERR_OUT_OF_RANGE` instead of being
+                 silently altered.
   - version:
      - v26.4.0
      - v24.19.0
@@ -2590,6 +2605,7 @@ console.log('listening on', server.address().port);
 [`'timeout'`]: #event-timeout
 [`BoundSocket`]: #class-netboundsocket
 [`ERR_INVALID_ARG_VALUE`]: errors.md#err_invalid_arg_value
+[`ERR_OUT_OF_RANGE`]: errors.md#err_out_of_range
 [`ERR_SOCKET_HANDLE_ADOPTED`]: errors.md#err_socket_handle_adopted
 [`EventEmitter`]: events.md#class-eventemitter
 [`child_process.fork()`]: child_process.md#child_processforkmodulepath-args-options

lib/net.js

@@ -169,6 +169,10 @@ const { kTimeout } = require('internal/timers');
 const DEFAULT_IPV4_ADDR = '0.0.0.0';
 const DEFAULT_IPV6_ADDR = '::';
 
+// uv_tcp_keepalive() takes its delays in seconds and rejects anything above
+// 32767, the largest value the socket options can carry.
+const KEEP_ALIVE_MAX_DELAY_MSECS = 32767 * 1000;
+
 const noop = () => {};
 
 const kPerfHooksNetConnectContext = Symbol('kPerfHooksNetConnectContext');
@@ -847,6 +851,20 @@ Socket.prototype.setNoDelay = function(enable) {
 };
 
 
+// The underlying socket options are configured in whole seconds, so a positive
+// value below 1000 ms would be truncated to 0 seconds and leave the system
+// default in place instead of applying the requested timing. Reject it rather
+// than applying something other than what was asked for. A non-positive value
+// keeps its documented meaning of leaving the current setting unchanged.
+// Agent passes Infinity to mean "no timeout", which also truncates to 0; it is
+// left alone here rather than given a new meaning.
+function validateKeepAliveDelay(msecs, name) {
+  if (msecs === undefined || msecs <= 0 || msecs === Infinity) {
+    return;
+  }
+  validateNumber(msecs, name, 1000, KEEP_ALIVE_MAX_DELAY_MSECS);
+}
+
 Socket.prototype.setKeepAlive = function(enable, initialDelayMsecs,
                                          intervalMsecs, count) {
   if (enable !== null && typeof enable === 'object') {
@@ -861,6 +879,11 @@ Socket.prototype.setKeepAlive = function(enable, initialDelayMsecs,
   const interval = intervalMsecs === undefined ?
     undefined : ~~(intervalMsecs / 1000);
 
+  if (enable) {
+    validateKeepAliveDelay(initialDelayMsecs, 'initialDelay');
+    validateKeepAliveDelay(intervalMsecs, 'interval');
+  }
+
   if (!this._handle) {
     this[kSetKeepAlive] = enable;
     this[kSetKeepAliveInitialDelay] = initialDelay;

test/parallel/test-async-hooks-http-parser-destroy.js

@@ -9,7 +9,7 @@ const http = require('http');
 // parser instances.
 
 const N = 50;
-const KEEP_ALIVE = 100;
+const KEEP_ALIVE = 1000;
 
 const createdIdsIncomingMessage = [];
 const createdIdsClientRequest = [];

test/parallel/test-net-keepalive-delay-range.js

@@ -0,0 +1,79 @@
+'use strict';
+
+require('../common');
+const assert = require('assert');
+const net = require('net');
+
+// The keep-alive delays are given in milliseconds but the underlying socket
+// options are configured in whole seconds, and uv_tcp_keepalive() rejects a
+// delay outside [1, 32767]. Verifies that a delay which cannot be applied as
+// requested is rejected instead of being silently altered.
+
+const MAX_DELAY = 32767 * 1000;
+
+// A positive delay below 1000 ms would be truncated to 0 seconds, which leaves
+// the system default in place instead of applying the requested timing.
+for (const initialDelay of [1, 400, 999]) {
+  assert.throws(() => net.Socket.prototype.setKeepAlive.call(
+    {}, true, initialDelay), {
+    code: 'ERR_OUT_OF_RANGE',
+    name: 'RangeError',
+    message: /The value of "initialDelay" is out of range/,
+  });
+}
+
+// The interval is validated the same way.
+assert.throws(() => net.Socket.prototype.setKeepAlive.call(
+  {}, true, 5000, 500), {
+  code: 'ERR_OUT_OF_RANGE',
+  message: /The value of "interval" is out of range/,
+});
+
+// A delay above the maximum cannot be carried by the socket options.
+assert.throws(() => net.Socket.prototype.setKeepAlive.call(
+  {}, true, MAX_DELAY + 1000), {
+  code: 'ERR_OUT_OF_RANGE',
+  message: /The value of "initialDelay" is out of range/,
+});
+
+// The options object form is validated as well.
+assert.throws(() => net.Socket.prototype.setKeepAlive.call(
+  {}, { enable: true, initialDelay: 999 }), {
+  code: 'ERR_OUT_OF_RANGE',
+  message: /The value of "initialDelay" is out of range/,
+});
+
+// A non-numeric delay is rejected by the same check.
+assert.throws(() => net.Socket.prototype.setKeepAlive.call(
+  {}, true, '1000'), {
+  code: 'ERR_INVALID_ARG_TYPE',
+});
+
+// Delays that can be applied as requested are accepted. A non-positive value
+// keeps its documented meaning of leaving the current setting unchanged, and
+// so does omitting it entirely.
+const server = net.createServer();
+server.listen(0, () => {
+  const client = net.connect({ port: server.address().port }, () => {
+    for (const args of [
+      [true, 1000],
+      [true, MAX_DELAY],
+      [true, 5000, 1000],
+      [true, 0],
+      [true, -1],
+      [true],
+      // Agent passes Infinity to mean "no timeout".
+      [true, Infinity],
+      [{ enable: true, initialDelay: 1000 }],
+      // Nothing is configured while keep-alive is disabled, so a delay that
+      // could not be applied is not rejected either.
+      [false, 400],
+    ]) {
+      client.setKeepAlive(...args);
+    }
+
+    client.end();
+  });
+
+  client.on('end', () => server.close());
+});

test/parallel/test-net-keepalive.js

@@ -38,8 +38,9 @@ const echoServer = net.createServer(common.mustCall((connection) => {
   }, 1), common.platformTimeout(100));
   connection.setTimeout(0);
   assert.notStrictEqual(connection.setKeepAlive, undefined);
-  // Send a keepalive packet after 50 ms
-  connection.setKeepAlive(true, common.platformTimeout(50));
+  // Send a keepalive packet after 1 second. Sub-second delays cannot be
+  // expressed by the underlying socket options.
+  connection.setKeepAlive(true, 1000);
   connection.on('end', function() {
     connection.end();
   });

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

@@ -27,7 +27,7 @@ echoServer.on('listening', common.mustCall(function() {
   clientConnection = new net.Socket();
   // Send a keepalive packet after 1000 ms
   // and make sure it persists
-  const s = clientConnection.setKeepAlive(true, 400);
+  const s = clientConnection.setKeepAlive(true, 1000);
   assert.ok(s instanceof net.Socket);
   clientConnection.connect(this.address().port);
   clientConnection.setTimeout(0);