net: warn on keep-alive delays truncated to zero

The keep-alive delays are given in milliseconds but the underlying
socket options are configured in whole seconds, so a positive value
below 1000 ms rounds down to 0. That leaves the system default in
place instead of applying the requested timing, and there is nothing
to indicate that the value had no effect.

Emit a KeepAliveWarning when a positive initialDelay or interval is
truncated to zero, and document the behaviour. A value of 0 keeps its
documented meaning of leaving the current setting unchanged and does
not warn.

Two existing tests passed values below 1000 ms that did not match what
their comments described; they now use 1000 ms.

Refs: #57712
Signed-off-by: Avocado <ujubongbong@gmail.com>

Author: zeexzeex

doc/api/net.md

@@ -1669,7 +1669,12 @@ 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. A positive value
+below `1000` therefore rounds down to `0`, which leaves the corresponding
+system default unchanged rather than applying the requested timing. Since
+this is rarely intended, a `KeepAliveWarning` process warning is emitted in
+that case. Sub-second timings cannot be expressed: use a value of at least
+`1000` milliseconds.
 
 Enabling the keep-alive functionality will set the following socket options:
 

lib/net.js

@@ -847,6 +847,20 @@ Socket.prototype.setNoDelay = function(enable) {
 };
 
 
+// The underlying socket options are configured in whole seconds, so a positive
+// value below 1000 ms is truncated to 0, which leaves the system default in
+// place instead of applying the requested timing. Warn so that this is not
+// silently ignored.
+function warnOnTruncatedKeepAlive(msecs, seconds, name) {
+  if (seconds === 0 && msecs > 0) {
+    process.emitWarning(
+      `The keep-alive ${name} of ${msecs} ms was truncated to 0 seconds and ` +
+      'has no effect. Use a value of at least 1000 ms.',
+      'KeepAliveWarning',
+    );
+  }
+}
+
 Socket.prototype.setKeepAlive = function(enable, initialDelayMsecs,
                                          intervalMsecs, count) {
   if (enable !== null && typeof enable === 'object') {
@@ -861,6 +875,11 @@ Socket.prototype.setKeepAlive = function(enable, initialDelayMsecs,
   const interval = intervalMsecs === undefined ?
     undefined : ~~(intervalMsecs / 1000);
 
+  if (enable) {
+    warnOnTruncatedKeepAlive(initialDelayMsecs, initialDelay, 'initialDelay');
+    warnOnTruncatedKeepAlive(intervalMsecs, interval, 'interval');
+  }
+
   if (!this._handle) {
     this[kSetKeepAlive] = enable;
     this[kSetKeepAliveInitialDelay] = initialDelay;

test/parallel/test-net-keepalive-truncation-warning.js

@@ -0,0 +1,108 @@
+'use strict';
+
+const common = 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. A positive value below 1000 ms is
+// truncated to 0 seconds, which leaves the system default in place instead of
+// applying the requested timing. Verifies that this emits a warning rather
+// than being silently ignored.
+
+// Warnings are emitted on the process, so the cases run one at a time to keep
+// each one's warnings from being observed by the others.
+const cases = [
+  // A delay below 1000 ms is truncated to 0 and warns.
+  {
+    configure: (client) => client.setKeepAlive(true, 400),
+    check: common.mustCall((messages) => {
+      assert.strictEqual(messages.length, 1);
+      assert.match(messages[0], /initialDelay of 400 ms/);
+      assert.match(messages[0], /at least 1000 ms/);
+    }),
+  },
+  // The interval is truncated the same way and warns independently.
+  {
+    configure: (client) => client.setKeepAlive(true, 5000, 500),
+    check: common.mustCall((messages) => {
+      assert.strictEqual(messages.length, 1);
+      assert.match(messages[0], /interval of 500 ms/);
+    }),
+  },
+  // Both delays can be truncated by the same call.
+  {
+    configure: (client) => client.setKeepAlive(true, 400, 500),
+    check: common.mustCall((messages) => {
+      assert.strictEqual(messages.length, 2);
+      assert.match(messages[0], /initialDelay of 400 ms/);
+      assert.match(messages[1], /interval of 500 ms/);
+    }),
+  },
+  // The options object form warns as well.
+  {
+    configure: (client) => client.setKeepAlive({
+      enable: true,
+      initialDelay: 999,
+    }),
+    check: common.mustCall((messages) => {
+      assert.strictEqual(messages.length, 1);
+      assert.match(messages[0], /initialDelay of 999 ms/);
+    }),
+  },
+  // A delay of at least 1000 ms is applied as requested and does not warn.
+  {
+    configure: (client) => client.setKeepAlive(true, 1000),
+    check: common.mustCall((messages) => assert.deepStrictEqual(messages, [])),
+  },
+  // 0 means "leave the current value unchanged" and is not a truncation.
+  {
+    configure: (client) => client.setKeepAlive(true, 0),
+    check: common.mustCall((messages) => assert.deepStrictEqual(messages, [])),
+  },
+  // Omitting the delay does not warn.
+  {
+    configure: (client) => client.setKeepAlive(true),
+    check: common.mustCall((messages) => assert.deepStrictEqual(messages, [])),
+  },
+  // Nothing is configured when keep-alive is disabled, so there is nothing to
+  // warn about.
+  {
+    configure: (client) => client.setKeepAlive(false, 400),
+    check: common.mustCall((messages) => assert.deepStrictEqual(messages, [])),
+  },
+];
+
+function runCase({ configure, check }, done) {
+  const messages = [];
+  const onWarning = (warning) => {
+    if (warning.name === 'KeepAliveWarning') messages.push(warning.message);
+  };
+  process.on('warning', onWarning);
+
+  const server = net.createServer();
+  server.listen(0, common.mustCall(() => {
+    const client = net.connect(
+      { port: server.address().port },
+      common.mustCall(() => {
+        configure(client);
+        client.end();
+      }));
+
+    client.on('end', common.mustCall(() => {
+      server.close(common.mustCall(() => {
+        // Warnings are emitted on the next tick.
+        setImmediate(() => {
+          process.removeListener('warning', onWarning);
+          check(messages);
+          done();
+        });
+      }));
+    }));
+  }));
+}
+
+(function next(i) {
+  if (i === cases.length) return;
+  runCase(cases[i], common.mustCall(() => next(i + 1)));
+})(0);

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. Values below 1000 ms are
+  // truncated to 0 seconds and would leave keep-alive unconfigured.
+  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);