diff --git a/doc/api/net.md b/doc/api/net.md index ab569a7b9c6f..5bfa20adbb1d 100644 --- a/doc/api/net.md +++ b/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: diff --git a/lib/net.js b/lib/net.js index d2b510c64bbb..dc9eb39b3639 100644 --- a/lib/net.js +++ b/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; diff --git a/test/parallel/test-net-keepalive-truncation-warning.js b/test/parallel/test-net-keepalive-truncation-warning.js new file mode 100644 index 000000000000..363adebabe5b --- /dev/null +++ b/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); diff --git a/test/parallel/test-net-keepalive.js b/test/parallel/test-net-keepalive.js index 26a4c5b3d648..86d1fb617940 100644 --- a/test/parallel/test-net-keepalive.js +++ b/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(); }); diff --git a/test/parallel/test-net-persistent-keepalive.js b/test/parallel/test-net-persistent-keepalive.js index 8caf640a4bbe..5dbc8560b35b 100644 --- a/test/parallel/test-net-persistent-keepalive.js +++ b/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);