From 8178098d3e85ad9bd13c536b504adf940ef08563 Mon Sep 17 00:00:00 2001 From: William Blankenship Date: Mon, 18 Mar 2019 13:42:54 -0700 Subject: [PATCH] feat(first): Handlers that execute ASAP in the req/res lifecycle (#1756) --- lib/server.js | 112 ++++++++++++++++++++++++++++++- test/server.test.js | 156 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 265 insertions(+), 3 deletions(-) diff --git a/lib/server.js b/lib/server.js index c89dc790a..e70f012b4 100644 --- a/lib/server.js +++ b/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 diff --git a/test/server.test.js b/test/server.test.js index 970b0a6db..ba19cbb55 100644 --- a/test/server.test.js +++ b/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); +});