From c6f61e6b564993b5476b04a804fa4b8706c35edf Mon Sep 17 00:00:00 2001 From: "P. Roebuck" Date: Fri, 9 Nov 2018 06:42:53 -0600 Subject: [PATCH] Prevent timeout value from skirting limit check (#3536) * fix(lib/runnable.js): Prevent timeout value from skirting limit check - Moved the timestring->value translation code to before the limit check. - Found that previous "fix" hadn't actually fixed the correct value, as the wrong upper bound value had been used (off by one error). - Further research indicated that some versions of IE had problems with negative timeout values. New code clamps to nonnegative numbers ending at `INT_MAX`. - Updated the function documentation. * feat(lib/utils.js): Add `clamp` function - New function clamps a numeric value to a given range. * test(unit/runnable.spec.js): Updated tests for `#timeout(ms)` - Restructured `Runnable#timeout` tests to check both numeric/timestamp input values that: - less than lower limit - equal to lower limit - within limits - equal to upper limit - greater than upper limit Closes #1652 --- lib/runnable.js | 36 ++++++++++++---- lib/utils.js | 11 +++++ test/unit/runnable.spec.js | 86 +++++++++++++++++++++++++++++++++----- 3 files changed, 115 insertions(+), 18 deletions(-) diff --git a/lib/runnable.js b/lib/runnable.js index 73da817793..2e62a6bc8b 100644 --- a/lib/runnable.js +++ b/lib/runnable.js @@ -50,23 +50,43 @@ function Runnable(title, fn) { utils.inherits(Runnable, EventEmitter); /** - * Set & get timeout `ms`. + * Get current timeout value in msecs. * - * @api private - * @param {number|string} ms - * @return {Runnable|number} ms or Runnable instance. + * @private + * @returns {number} current timeout threshold value + */ +/** + * @summary + * Set timeout threshold value (msecs). + * + * @description + * A string argument can use shorthand (e.g., "2s") and will be converted. + * The value will be clamped to range [0, 2^31-1]. + * If clamped value matches either range endpoint, timeouts will be disabled. + * + * @private + * @see {@link https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/setTimeout#Maximum_delay_value} + * @param {number|string} ms - Timeout threshold value. + * @returns {Runnable} this + * @chainable */ Runnable.prototype.timeout = function(ms) { if (!arguments.length) { return this._timeout; } - // see #1652 for reasoning - if (ms === 0 || ms > Math.pow(2, 31)) { - this._enableTimeouts = false; - } if (typeof ms === 'string') { ms = milliseconds(ms); } + + // Clamp to range + var INT_MAX = Math.pow(2, 31) - 1; + var range = [0, INT_MAX]; + ms = utils.clamp(ms, range); + + // see #1652 for reasoning + if (ms === range[0] || ms === range[1]) { + this._enableTimeouts = false; + } debug('timeout %d', ms); this._timeout = ms; if (this.timer) { diff --git a/lib/utils.js b/lib/utils.js index 71ea6ce4fb..6e78a99b71 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -691,6 +691,17 @@ exports.isPromise = function isPromise(value) { return typeof value === 'object' && typeof value.then === 'function'; }; +/** + * Clamps a numeric value to an inclusive range. + * + * @param {number} value - Value to be clamped. + * @param {numer[]} range - Two element array specifying [min, max] range. + * @returns {number} clamped value + */ +exports.clamp = function clamp(value, range) { + return Math.min(Math.max(value, range[0]), range[1]); +}; + /** * It's a noop. * @api diff --git a/test/unit/runnable.spec.js b/test/unit/runnable.spec.js index 9ea00b6d07..0fb29db6e5 100644 --- a/test/unit/runnable.spec.js +++ b/test/unit/runnable.spec.js @@ -46,18 +46,84 @@ describe('Runnable(title, fn)', function() { }); describe('#timeout(ms)', function() { - it('should set the timeout', function() { - var run = new Runnable(); - run.timeout(1000); - assert(run.timeout() === 1000); + var MIN_TIMEOUT = 0; + var MAX_TIMEOUT = 2147483647; // INT_MAX (32-bit signed integer) + + describe('when value is less than lower bound', function() { + it('should clamp to lower bound given numeric', function() { + var run = new Runnable(); + run.timeout(-1); + assert(run.timeout() === MIN_TIMEOUT); + }); + // :TODO: Our internal version of `ms` can't handle negative time, + // but package version can. Skip this check until that change is merged. + it.skip('should clamp to lower bound given timestamp', function() { + var run = new Runnable(); + run.timeout('-1 ms'); + assert(run.timeout() === MIN_TIMEOUT); + }); }); - }); - describe('#timeout(ms) when ms>2^31', function() { - it('should set disabled', function() { - var run = new Runnable(); - run.timeout(1e10); - assert(run.enableTimeouts() === false); + describe('when value is equal to lower bound', function() { + it('should set the value and disable timeouts given numeric', function() { + var run = new Runnable(); + run.timeout(MIN_TIMEOUT); + assert(run.timeout() === MIN_TIMEOUT); + assert(run.enableTimeouts() === false); + }); + it('should set the value and disable timeouts given timestamp', function() { + var run = new Runnable(); + run.timeout(MIN_TIMEOUT + 'ms'); + assert(run.timeout() === MIN_TIMEOUT); + assert(run.enableTimeouts() === false); + }); + }); + + describe('when value is within `setTimeout` bounds', function() { + var oneSecond = 1000; + + it('should set the timeout given numeric', function() { + var run = new Runnable(); + run.timeout(oneSecond); + assert(run.timeout() === oneSecond); + assert(run.enableTimeouts() === true); + }); + it('should set the timeout given timestamp', function() { + var run = new Runnable(); + run.timeout('1s'); + assert(run.timeout() === oneSecond); + assert(run.enableTimeouts() === true); + }); + }); + + describe('when value is equal to upper bound', function() { + it('should set the value and disable timeout given numeric', function() { + var run = new Runnable(); + run.timeout(MAX_TIMEOUT); + assert(run.timeout() === MAX_TIMEOUT); + assert(run.enableTimeouts() === false); + }); + it('should set the value and disable timeout given timestamp', function() { + var run = new Runnable(); + run.timeout(MAX_TIMEOUT + 'ms'); + assert(run.timeout() === MAX_TIMEOUT); + assert(run.enableTimeouts() === false); + }); + }); + + describe('when value is greater than `setTimeout` limit', function() { + it('should clamp to upper bound given numeric', function() { + var run = new Runnable(); + run.timeout(MAX_TIMEOUT + 1); + assert(run.timeout() === MAX_TIMEOUT); + assert(run.enableTimeouts() === false); + }); + it('should clamp to upper bound given timestamp', function() { + var run = new Runnable(); + run.timeout('24.9d'); // 2151360000ms + assert(run.timeout() === MAX_TIMEOUT); + assert(run.enableTimeouts() === false); + }); }); });