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
Create inflightRequestThrottle plugin #1431
Changes from 5 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -153,6 +153,13 @@ event, e.g., `server.on('after', plugins.metrics());`: | |
* `res` {Object} the response obj | ||
* `route` {Object} the route obj that serviced the request | ||
|
||
The module includes the following plugins to be used with restify's `pre` event: | ||
* `inflightRequestThrottle(options)` - limits the max number of inflight requests | ||
* `options.limit` {Number} the maximum number of inflight requests the server will handle before returning an error | ||
* `options.res` {Error} An error that will be passed to `res.send` when the limit is reached. | ||
* `options.res.statusCode` {Number} The status code to return when the limit is reached. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In terms of StatusCode here -- I almost think we should have the user just pass in a restify error of their choosing, and eliminate having to side channel the status code here. If they want to override the status code, they can just pass in an error they construct which contains the statuscode. |
||
* `options.server` {Object} The restify server that this module will throttle | ||
|
||
## Accept Parser | ||
|
||
Parses out the `Accept` header, and ensures that the server can respond to what | ||
|
@@ -439,6 +446,28 @@ uniform request distribution. To enable this, you can pass in | |
`options.tokensTable`, which is simply any Object that supports `put` and `get` | ||
with a `String` key, and an `Object` value. | ||
|
||
## Inflight Request Throttling | ||
|
||
```js | ||
var errors = require('restify-errors'); | ||
var restify = require('restify'); | ||
|
||
var server = restify.createServer(); | ||
const options = { limit: 600, server: server }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
options.res = new errors.InternalServerError(); | ||
server.pre(restify.plugins.inflightRequestThrottle(options)); | ||
``` | ||
|
||
The `inflightRequestThrottle` module allows you to specify an upper limit to | ||
the maximum number of inflight requests your server is able to handle. This | ||
is a simple heuristic for protecting against event loop contention between | ||
requests causing unacceptable latencies. | ||
|
||
The custom response is optional, and allows you to specify your own response | ||
and status code when rejecting incoming requests due to too many inflight | ||
requests. It defaults to `503 ServiceUnavailableError`. | ||
|
||
|
||
## Conditional Request Handler | ||
|
||
```js | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
'use strict'; | ||
|
||
var assert = require('assert-plus'); | ||
var ServiceUnavailableError = require('restify-errors').ServiceUnavailableError; | ||
var defaultResponse = new ServiceUnavailableError('resource exhausted'); | ||
|
||
/** | ||
* inflightRequestThrottle | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You may want to specify both here and in docs that users should register this plugin first as order matters and you want the throttle check to occur as early as possible. |
||
* | ||
* @param {Object} opts configure this plugin | ||
* @param {Number} opts.limit maximum number of simultanenous | ||
* connections the server will handle before returning an error | ||
* @param {Error} opts.res the error object to pass to res.send when the | ||
* maximum concurrency is exceeded | ||
* @param {Number} opts.res.statusCode the status code to return when the | ||
* maximum concurrency is exceeded | ||
* @param {Function} opts.server the instance of the restify server this module | ||
* will throttle. | ||
* @returns {Function} middleware to be registered on server.pre | ||
*/ | ||
function inflightRequestThrottle (opts) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this class need a constructor? Generally, the pattern for plugins in restify has been to use a closure, and obviating the need for a constructor. Can we keep it consistent and do that here? We can of course, debate closure vs constructor trade offs at another point in time. Here's an example: https://github.com/restify/node-restify/blob/5.x/lib/plugins/metrics.js#L23 |
||
|
||
// Scrub input and populate our configuration | ||
assert.object(opts, 'opts'); | ||
assert.number(opts.limit, 'opts.limit'); | ||
assert.object(opts.server, 'opts.server'); | ||
assert.func(opts.server.inflightRequests, 'opts.server.inflightRequests'); | ||
|
||
if (opts.res !== undefined && opts.res !== null) { | ||
assert.ok(opts.res instanceof Error, 'opts.res must be an error'); | ||
assert.optionalNumber(opts.res.statusCode, 'opts.res.statusCode'); | ||
} | ||
|
||
var self = {}; | ||
self._res = opts.res || defaultResponse; | ||
self._limit = opts.limit; | ||
self._server = opts.server; | ||
|
||
function onRequest (req, res, next) { | ||
var inflightRequests = self._server.inflightRequests(); | ||
|
||
if (inflightRequests > self._limit) { | ||
req.log.trace({ | ||
plugin: 'inflightRequestThrottle', | ||
inflightRequests: inflightRequests, | ||
limit: self._limit | ||
}, 'maximum inflight requests exceeded, rejecting request'); | ||
return res.send(self._res); | ||
} | ||
|
||
return next(); | ||
} | ||
|
||
// We need to bind in order to keep our `this` context when passed back | ||
// out of the constructor. | ||
return onRequest; | ||
} | ||
|
||
inflightRequestThrottle.prototype.onRequest = | ||
|
||
module.exports = inflightRequestThrottle; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,119 @@ | ||
'use strict'; | ||
|
||
var assert = require('chai').assert; | ||
var restify = require('../../lib/index.js'); | ||
var restifyClients = require('restify-clients'); | ||
var P = restify.plugins.inflightRequestThrottle; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why P? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line length in the tests, it stands for |
||
|
||
function fakeServer(count) { | ||
return { | ||
inflightRequests: function () { | ||
return count; | ||
} | ||
}; | ||
} | ||
|
||
describe('inlfightRequestThrottle', function () { | ||
|
||
it('Unit: Should shed load', function (done) { | ||
var logged = false; | ||
var p = P({ server: fakeServer(10), limit: 1 }); | ||
function send (body) { | ||
assert(logged, 'Should have emitted a log'); | ||
assert.equal(body.statusCode, 503, 'Defaults to 503 status'); | ||
assert(body instanceof Error, 'Defaults to error body'); | ||
done(); | ||
} | ||
function next () { | ||
assert(false, 'Should not call next'); | ||
done(); | ||
} | ||
function trace () { | ||
logged = true; | ||
} | ||
var log = { trace: trace }; | ||
var fakeReq = { log: log }; | ||
p(fakeReq, { send: send }, next); | ||
}); | ||
|
||
it('Unit: Should support custom response', function (done) { | ||
var server = fakeServer(10); | ||
var res = new Error('foo'); | ||
var p = P({ server: server, limit: 1, res: res }); | ||
function send (body) { | ||
assert.equal(body, res, 'Overrides body'); | ||
done(); | ||
} | ||
function next () { | ||
assert(false, 'Should not call next'); | ||
done(); | ||
} | ||
var fakeReq = { log : { trace: function () {} } }; | ||
p(fakeReq, { send: send }, next); | ||
}); | ||
|
||
it('Unit: Should let request through when not under load', function (done) { | ||
var p = P({ server: fakeServer(1), limit: 2 }); | ||
function send () { | ||
assert(false, 'Should not call send'); | ||
done(); | ||
} | ||
function next () { | ||
assert(true, 'Should call next'); | ||
done(); | ||
} | ||
var fakeReq = { log : { trace: function () {} } }; | ||
p(fakeReq, { send: send }, next); | ||
}); | ||
|
||
it('Integration: Should shed load', function (done) { | ||
var server = restify.createServer(); | ||
var client = { | ||
close: function () {} | ||
}; | ||
var isDone = false; | ||
var to; | ||
function finish() { | ||
if (isDone) { | ||
return null; | ||
} | ||
clearTimeout(to); | ||
isDone = true; | ||
client.close(); | ||
server.close(); | ||
return done(); | ||
} | ||
to = setTimeout(finish, 2000); | ||
var err = new Error('foo'); | ||
err.statusCode = 555; | ||
server.pre(P({ server: server, limit: 1, res: err })); | ||
var RES; | ||
server.get('/foo', function (req, res) { | ||
if (RES) { | ||
res.send(999); | ||
} else { | ||
RES = res; | ||
} | ||
}); | ||
server.listen(0, '127.0.0.1', function () { | ||
client = restifyClients.createJsonClient({ | ||
url: 'http://127.0.0.1:' + server.address().port, | ||
retry: false | ||
}); | ||
client.get({ path: '/foo' }, function (e, _, res) { | ||
assert(e === null || e === undefined, | ||
'First request isnt shed'); | ||
assert.equal(res.statusCode, 200, '200 returned on success'); | ||
finish(); | ||
}); | ||
client.get({ path: '/foo' }, function (e, _, res) { | ||
assert(e, 'Second request is shed'); | ||
assert.equal(e.name, | ||
'InternalServerError', 'Default err returned'); | ||
assert.equal(res.statusCode, 555, | ||
'Default shed status code returned'); | ||
RES.send(200); | ||
}); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to go one more time on this -- now that I understand this is an actual error, can we just name it options.err instead? That makes it a lot more clear, at least to me. Thoughts? And totally my bad for not realizing this in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed,
options.res
doesn't feel intuitive.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, I totally agree. This grew organically since the original implementation expected this to be an array of parameters that would be passed to
res.send.apply
. Now that we expect a restify error, its an awkward fit.