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

@@ -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 [<code>0</code>, <code>2^<sup>31</sup>-1</code>].
+ * 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) {

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

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);
+      });
     });
   });