net: warn on keep-alive delays truncated to zero

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: (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: (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: (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: (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: (messages) => assert.deepStrictEqual(messages, []),
+  },
+  // 0 means "leave the current value unchanged" and is not a truncation.
+  {
+    configure: (client) => client.setKeepAlive(true, 0),
+    check: (messages) => assert.deepStrictEqual(messages, []),
+  },
+  // Omitting the delay does not warn.
+  {
+    configure: (client) => client.setKeepAlive(true),
+    check: (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: (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);