Create inflightRequestThrottle plugin#1431
Conversation
docs/api/plugins.md
Outdated
|
|
||
| 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 simultaneous connections the server will handle before returning an error |
There was a problem hiding this comment.
Second this -- let's keep it consistent :)
docs/api/plugins.md
Outdated
| ```js | ||
| const opts = { limit: 600, server: server }; | ||
| opts.error = new Error('Resource Exhausted'); | ||
| error.statuscode = 420; |
There was a problem hiding this comment.
Should that be camelCase statusCode? Also, I might suggest adding an example using restify-errors constructors.
docs/api/plugins.md
Outdated
|
|
||
| ```js | ||
| const opts = { limit: 600, server: server }; | ||
| opts.error = new Error('Resource Exhausted'); |
There was a problem hiding this comment.
Is it opts.error or opts.resp?
| plugin: 'inflightRequestThrottle', | ||
| inflightRequests: inflightRequests, | ||
| limit: self._limit, | ||
| message: 'maximum inflight requests exceeded, rejecting request' |
There was a problem hiding this comment.
Whoops, sorry, I should have been clearer about my last comment on this. I meant using the logger API itself, so something like this:
req.log.trace({
plugin: 'inflightRequestThrottle',
inflightRequests: inflightRequests,
limit: self._limit
}, 'maximum inflight requests exceeded, rejecting request');
|
@DonutEspresso one more time? |
docs/api/plugins.md
Outdated
|
|
||
| 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 simultaneous connections the server will handle before returning an error |
There was a problem hiding this comment.
s/simultaneous connections/inflight requests.
docs/api/plugins.md
Outdated
| 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 simultaneous connections the server will handle before returning an error | ||
| * `options.resp` {Error} An error that will be passed to `res.send` when the limit is reached. |
There was a problem hiding this comment.
what does resp stand for? Response? If so can use the convention of res?
| ## Inflight Request Throttling | ||
|
|
||
| ```js | ||
| const options = { limit: 600, server: server }; |
There was a problem hiding this comment.
The server property is not in docs. Do we need to pass in server or should we be only passing in the specific properties that the throttle plugin needs?
docs/api/plugins.md
Outdated
|
|
||
| ```js | ||
| const options = { limit: 600, server: server }; | ||
| options.resp = new require('restify-errors').InternalServerError(); |
There was a problem hiding this comment.
Since people copy paste snippets all the time, we should hoist the require up to the top and then call new on the constructor. Additionally, I thought this was a response? Why is it now an error object?
There was a problem hiding this comment.
Errors get serialized by res.__send
lib/plugins/index.js
Outdated
| serveStatic: require('./static'), | ||
| throttle: require('./throttle'), | ||
| urlEncodedBodyParser: require('./formBodyParser'), | ||
| inflightRequestThrottle: require('./inflightRequestThrottle'), |
There was a problem hiding this comment.
We generally sort these alphabetically -- so please do that. And yes it'd be awesome if we had a lint rule for this, but we don't yet :(
| * server. | ||
| * @returns {Function} middleware to be registered on server.pre | ||
| */ | ||
| function inflightRequestThrottle (opts) { |
There was a problem hiding this comment.
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
|
@yunong one more time? |
docs/api/plugins.md
Outdated
| 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. |
There was a problem hiding this comment.
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.
Agreed, options.res doesn't feel intuitive.
There was a problem hiding this comment.
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.
| 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.
Line length in the tests, it stands for Plugin. I will try something else though, single letter variables feel wrong.
docs/api/plugins.md
Outdated
| * `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.
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.
| var defaultResponse = new ServiceUnavailableError('resource exhausted'); | ||
|
|
||
| /** | ||
| * inflightRequestThrottle |
There was a problem hiding this comment.
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.
|
@yunong one more time? |
|
❤️ |
4 participants
Plugin for limiting inflight requests