Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 17 commits into from Mar 18, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
112 changes: 109 additions & 3 deletions lib/server.js
Expand Up @@ -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
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we dropped Node6, any reason not to use rest parameters here?

assert.arrayOfFunc(...func);
this.earliestChain = this.earliestChain.concat(...func);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linting fails.

I originally used const and let everywhere but make prepush failed, so I changed everything back to good ol' ES5.

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
Expand Down Expand Up @@ -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
Expand Down
156 changes: 156 additions & 0 deletions test/server.test.js
Expand Up @@ -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);
});