feat(first): Handlers that execute ASAP in the req/res lifecycle (#1756)

lib/server.js

@@ -117,6 +117,7 @@ function Server(options) {
 
     this.onceNext = !!options.onceNext;
     this.strictNext = !!options.strictNext;
+    this.firstChain = [];
     this.preChain = new Chain({
         onceNext: this.onceNext,
         strictNext: this.strictNext
@@ -489,6 +490,73 @@ Server.prototype.pre = function pre() {
     return this;
 };
 
+// eslint-disable-next-line jsdoc/check-param-names
+/**
+ * Gives you hooks that run before restify touches a request. These hooks
+ * allow you to do processing early in the request/response life cycle without
+ * the overhead of the restify framework. You can not yield the event loop in
+ * this handler.
+ *
+ * The function handler accepts two parameters: req, res. If you want restify
+ * to ignore this request, return false from your handler. Return true or
+ * undefined to let restify continue handling the request.
+ *
+ * When false is returned, restify immediately stops handling the request. This
+ * means that no further middleware will be executed for any chain and routing
+ * will not occure. All request/response handling for an incoming request must
+ * be done inside the first handler if you intend to return false. This
+ * includes things like closing the response and returning a status code.
+ *
+ * The only work restify does for a first handler is to increment the number of
+ * inflightRequests prior to calling the chain, and decrement that value if the
+ * handler returns false. Returning anything other than true, false, undefined,
+ * or null will cause an exception to be thrown.
+ *
+ * Since server.first is designed to bypass the restify framework, there are
+ * naturally trade-offs you make when using this API:
+ *  * Standard restify lifecycle events such as 'after' are not triggered for
+ *    any request that you return false from a handler for
+ *  * Invoking any of the restify req/res APIs from within a first handler is
+ *    unspecified behavior, as the restify framework hasn't built up state for
+ *    the request yet.
+ *  * There are no request timers available at the time that the first chain
+ *    runs.
+ *  * And more! Beware doing anything with restify in these handlers. They are
+ *    designed to give you similar access to the req/res as you would have if
+ *    you were directly using node.js' http module, they are outside of the
+ *    restify framework!
+ *
+ * @public
+ * @memberof Server
+ * @instance
+ * @function first
+ * @param {...Function} handler - Allows you to add handlers that
+ * run for all requests, *before* restify touches the request.
+ * This gives you a hook to change request headers and the like if you need to.
+ * Note that `req.params` will be undefined, as that's filled in *after*
+ * routing.
+
+ * Takes one or more functions.
+ * @returns {Object} returns self
+ * @example
+ * server.first(function(req, res) {
+ *   if(server.inflightRequests() > 100) {
+ *     res.statusCode = 503;
+ *     res.end();
+ *     return false
+ *   }
+ *   return true;
+ * })
+ */
+Server.prototype.first = function first() {
+    var args = Array.prototype.slice.call(arguments);
+    for (var i = 0; i < args.length; i++) {
+        assert.func(args[i]);
+        this.firstChain.push(args[i]);
+    }
+    return this;
+};
+
 // eslint-disable-next-line jsdoc/check-param-names
 /**
  * Allows you to add in handlers that run for all routes. Note that handlers
@@ -807,19 +875,57 @@ Server.prototype.toString = function toString() {
 Server.prototype._onRequest = function _onRequest(req, res) {
     var self = this;
 
+    // Increment the number of inflight requests prior to calling the firstChain
+    // handlers. This accomplishes two things. First, it gives earliest an
+    // accurate count of how many inflight requests there would be including
+    // this new request. Second, it intentionally winds up the inflight request
+    // accounting with the implementation of firstChain. Note how we increment
+    // here, but decrement down inside the for loop below. It's easy to end up
+    // with race conditions betwen inflight request accounting and inflight
+    // request load shedding, causing load shedding to reject/allow too many
+    // requests. The current implementation of firstChain is designed to
+    // remove those race conditions. By winding these implementations up with
+    // one another, it makes it clear that moving around the inflight request
+    // accounting will change the behavior of earliest.
+    self._inflightRequests++;
+
+    // Give the first chain the earliest possible opportunity to process
+    // this request before we do any work on it.
+    var firstChain = self.firstChain;
+    for (var i = 0; i < firstChain.length; i++) {
+        var handle = firstChain[i](req, res);
+        // Limit the range of values we will accept as return results of
+        // first handlers. This helps us maintain forward compatibility by
+        // ensuring users don't rely on undocumented/unspecified behavior.
+        assert.ok(
+            handle === true ||
+                handle === false ||
+                handle === undefined ||
+                handle === null,
+            'Return value of first[' +
+                i +
+                '] must be: ' +
+                'boolean, undefined, or null'
+        );
+        // If the first handler returns false, stop handling the request
+        // immediately.
+        if (handle === false) {
+            self._inflightRequests--;
+            return;
+        }
+    }
+
     this.emit('request', req, res);
 
     // Skip Socket.io endpoints
     if (this.socketio && /^\/socket\.io.*/.test(req.url)) {
+        self._inflightRequests--;
         return;
     }
 
     // Decorate req and res objects
     self._setupRequest(req, res);
 
-    // increment number of requests
-    self._inflightRequests++;
-
     // Run in domain to catch async errors
     // It has significant negative performance impact
     // Warning: this feature depends on the deprecated domains module

test/server.test.js

@@ -2683,3 +2683,159 @@ test('should have proxy event handlers as instance', function(t) {
         t.end();
     });
 });
+
+test('first chain should get to reject requests', function(t) {
+    SERVER.get('/foobar', function(req, res, next) {
+        t.fail('should not call handler');
+    });
+
+    SERVER.first(function(req, res) {
+        res.statusCode = 413; // I'm a teapot!
+        res.end();
+        return false;
+    });
+
+    CLIENT.get('/foobar', function(_, __, res) {
+        t.equal(res.statusCode, 413);
+        t.end();
+    });
+});
+
+test('first chain should get to allow requests', function(t) {
+    SERVER.get('/foobar', function(req, res, next) {
+        res.send(413, 'Im a teapot');
+        return next();
+    });
+
+    SERVER.first(function(req, res) {
+        return true;
+    });
+
+    CLIENT.get('/foobar', function(_, __, res) {
+        t.equal(res.statusCode, 413);
+        t.end();
+    });
+});
+
+test('first chain should allow multiple handlers', function(t) {
+    SERVER.get('/foobar', function(req, res, next) {
+        res.send(413, 'Im a teapot');
+        return next();
+    });
+
+    var count = 0;
+    var handler = function() {
+        count++;
+    };
+
+    SERVER.first(handler, handler, handler);
+    SERVER.first(handler, handler, handler);
+
+    CLIENT.get('/foobar', function(_, __, res) {
+        t.equal(res.statusCode, 413);
+        t.equal(count, 6, 'invoked 6 handlers');
+        t.end();
+    });
+});
+
+test('first chain should allow any handler to reject', function(t) {
+    SERVER.get('/foobar', function(req, res, next) {
+        res.send(200, 'Handled');
+        return next();
+    });
+
+    var count = 0;
+    var handler = function() {
+        count++;
+    };
+
+    var handlerAbort = function(req, res) {
+        count++;
+        res.statusCode = 413;
+        res.end();
+        return false;
+    };
+
+    SERVER.first(handler, handler, handler);
+    // Should append these handlers and abort the chain on the second
+    SERVER.first(handler, handlerAbort, handler);
+    // These should never run
+    SERVER.first(handler, handlerAbort);
+
+    CLIENT.get('/foobar', function(_, __, res) {
+        t.equal(res.statusCode, 413);
+        t.equal(count, 5, 'invoked 5 handlers');
+        t.end();
+    });
+});
+
+test('inflightRequest accounting stable with firstChain', function(t) {
+    // Make 3 requests, shed the second, and ensure inflightRequest accounting
+    // for all the requests
+    var request = 0;
+    SERVER.first(function(req, res) {
+        request++;
+
+        if (request === 1) {
+            t.equal(SERVER._inflightRequests, 1);
+            return true;
+        }
+        if (request === 2) {
+            t.equal(SERVER._inflightRequests, 2);
+            res.statusCode = 413;
+            res.end();
+            return false;
+        }
+        if (request === 3) {
+            // Since the second request was shed, and inflightRequest accounting
+            // should be happening synchronously, this should still be 2 for
+            // the third request
+            t.equal(SERVER._inflightRequests, 2);
+            return true;
+        }
+
+        t.fail('Too many requests for test');
+        return false;
+    });
+    var nexts = [];
+    SERVER.get('/foobar', function(req, res, next) {
+        res.send(200, 'success');
+        nexts.push(next);
+        if (nexts.length === 2) {
+            nexts.forEach(function(finishRequest) {
+                finishRequest();
+            });
+        }
+    });
+
+    var results = [];
+    function getDone(_, __, res) {
+        results.push(res);
+        if (results.length < 3) {
+            return;
+        }
+        for (var i = 0; i < results.length; i++) {
+            // The shed request should always be returned first, since it isn't
+            // handled by SERVER.get
+            if (i === 0) {
+                t.equal(
+                    results[i].statusCode,
+                    413,
+                    'results[' + i + '] === 413'
+                );
+            } else {
+                t.equal(
+                    results[i].statusCode,
+                    200,
+                    'results[' + i + '] === 200'
+                );
+            }
+        }
+        t.end();
+    }
+
+    // kick off all 3 at the same time to see if we can trigger a race condition
+    CLIENT.get('/foobar', getDone);
+    CLIENT.get('/foobar', getDone);
+    CLIENT.get('/foobar', getDone);
+});