Create inflightRequestThrottle plugin #1431

retrohacker · 2017-08-03T17:42:26Z

Plugin for limiting inflight requests

DonutEspresso · 2017-08-03T20:55:01Z

docs/api/plugins.md

@@ -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 simultaneous connections the server will handle before returning an error


options vs opts?

Second this -- let's keep it consistent :)

DonutEspresso · 2017-08-03T20:56:22Z

docs/api/plugins.md

+```js
+const opts = { limit: 600, server: server };
+opts.error = new Error('Resource Exhausted');
+error.statuscode = 420;


Should that be camelCase statusCode? Also, I might suggest adding an example using restify-errors constructors.

DonutEspresso · 2017-08-03T20:57:33Z

docs/api/plugins.md

+
+```js
+const opts = { limit: 600, server: server };
+opts.error = new Error('Resource Exhausted');


Is it opts.error or opts.resp?

DonutEspresso · 2017-08-03T20:58:41Z

lib/plugins/inflightRequestThrottle.js

+            plugin: 'inflightRequestThrottle',
+            inflightRequests: inflightRequests,
+            limit: self._limit,
+            message: 'maximum inflight requests exceeded, rejecting request'


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');

retrohacker · 2017-08-03T21:07:41Z

@DonutEspresso one more time?

yunong · 2017-08-03T21:37:30Z

docs/api/plugins.md

@@ -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 simultaneous connections the server will handle before returning an error


s/simultaneous connections/inflight requests.

yunong · 2017-08-03T21:38:11Z

docs/api/plugins.md

+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.


what does resp stand for? Response? If so can use the convention of res?

yunong · 2017-08-03T21:39:09Z

docs/api/plugins.md

+## Inflight Request Throttling
+
+```js
+const options = { limit: 600, server: server };


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?

yunong · 2017-08-03T21:44:11Z

docs/api/plugins.md

+
+```js
+const options = { limit: 600, server: server };
+options.resp = new require('restify-errors').InternalServerError();


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?

Errors get serialized by res.__send

yunong · 2017-08-03T21:48:17Z

lib/plugins/index.js

@@ -25,6 +25,7 @@ module.exports = {
    serveStatic: require('./static'),
    throttle: require('./throttle'),
    urlEncodedBodyParser: require('./formBodyParser'),
+    inflightRequestThrottle: require('./inflightRequestThrottle'),


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 :(

yunong · 2017-08-03T21:50:10Z

lib/plugins/inflightRequestThrottle.js

+ *    server.
+ * @returns {Function} middleware to be registered on server.pre
+ */
+function inflightRequestThrottle (opts) {


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

retrohacker · 2017-08-03T22:16:44Z

@yunong one more time?

yunong · 2017-08-03T22:45:35Z

docs/api/plugins.md

+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.


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.

Agreed, options.res doesn't feel intuitive.

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.

yunong · 2017-08-03T22:46:33Z

test/plugins/inflightRequestThrottle.test.js

+var assert = require('chai').assert;
+var restify = require('../../lib/index.js');
+var restifyClients = require('restify-clients');
+var P = restify.plugins.inflightRequestThrottle;


Line length in the tests, it stands for Plugin. I will try something else though, single letter variables feel wrong.

yunong · 2017-08-03T22:47:35Z

docs/api/plugins.md

+* `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.


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.

yunong · 2017-08-04T18:29:18Z

lib/plugins/inflightRequestThrottle.js

+var defaultResponse = new ServiceUnavailableError('resource exhausted');
+
+/**
+ * inflightRequestThrottle


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.

retrohacker · 2017-08-04T20:34:08Z

@yunong one more time?

yunong

Looks good, nice work 👍

retrohacker · 2017-08-07T17:55:44Z

❤️

Create inflightRequestThrottle plugin

6c33f6e

retrohacker requested review from yunong and DonutEspresso August 3, 2017 17:42

DonutEspresso requested changes Aug 3, 2017

View reviewed changes

impl review

b1a62fe

linting

07de664

DonutEspresso approved these changes Aug 3, 2017

View reviewed changes

yunong requested changes Aug 3, 2017

View reviewed changes

retrohacker added 2 commits August 3, 2017 15:13

impl feedback

d2ad65c

impl feedback

11aefc2

yunong requested changes Aug 3, 2017

View reviewed changes

yunong reviewed Aug 4, 2017

View reviewed changes

retrohacker added 2 commits August 4, 2017 12:11

impl feedback

91568a6

impl feedback

2ab907a

yunong approved these changes Aug 4, 2017

View reviewed changes

retrohacker merged commit 285faf4 into 5.x Aug 7, 2017

retrohacker deleted the inflightRequestThrottle branch August 17, 2017 00:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create inflightRequestThrottle plugin #1431

Create inflightRequestThrottle plugin #1431

retrohacker commented Aug 3, 2017

DonutEspresso Aug 3, 2017

yunong Aug 3, 2017

DonutEspresso Aug 3, 2017

DonutEspresso Aug 3, 2017

DonutEspresso Aug 3, 2017

retrohacker commented Aug 3, 2017

yunong Aug 3, 2017

yunong Aug 3, 2017

yunong Aug 3, 2017

yunong Aug 3, 2017

retrohacker Aug 3, 2017

yunong Aug 3, 2017 •

edited

Loading

yunong Aug 3, 2017

retrohacker commented Aug 3, 2017

yunong Aug 3, 2017

sean3z Aug 4, 2017

retrohacker Aug 4, 2017

yunong Aug 3, 2017

retrohacker Aug 4, 2017

yunong Aug 3, 2017

yunong Aug 4, 2017

retrohacker commented Aug 4, 2017

yunong left a comment

retrohacker commented Aug 7, 2017

Create inflightRequestThrottle plugin #1431

Create inflightRequestThrottle plugin #1431

Conversation

retrohacker commented Aug 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

retrohacker commented Aug 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yunong Aug 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

retrohacker commented Aug 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

retrohacker commented Aug 4, 2017

yunong left a comment

Choose a reason for hiding this comment

retrohacker commented Aug 7, 2017

yunong Aug 3, 2017 •

edited

Loading