new: add inflightRequests() API #181
Conversation
README.md
Outdated
| @@ -470,6 +471,25 @@ client.get(options, function(err, res, socket, head) { | |||
| }); | |||
| ``` | |||
|
|
|||
| ### More APIs | |||
There was a problem hiding this comment.
What about "Common APIs" instead of "More APIs"?
README.md
Outdated
| Returns the number of inflight requests the client is currently handling. An | ||
| inflight request is a request that is in any of the following states: | ||
|
|
||
| * connection establishment (dns resolution, socket connection, etc.) |
There was a problem hiding this comment.
Should we add an item "* After the verb function is called but before any I/O (dns resolution, socket connection, etc.) is performed."? It's mentioned below but here it seems this list is meant to be exhaustive, so it seems it'd be misleading to not mention this state as part of the included states.
lib/HttpClient.js
Outdated
| @@ -268,8 +268,8 @@ function rawRequest(opts, cb) { | |||
| req.getMetrics = function getMetrics() { | |||
| return metrics; | |||
| }; | |||
| opts.client._inflightRequests--; | |||
There was a problem hiding this comment.
Should this be performed by a function that also checks (asserts?) that the number of in-flight requests is e.g. >= 0? I don't think we'd want to check for an upper bound.
lib/HttpClient.js
Outdated
| @@ -788,6 +793,9 @@ HttpClient.prototype.request = function request(opts, cb) { | |||
| assert.object(opts, 'options'); | |||
| assert.func(cb, 'callback'); | |||
|
|
|||
| var self = this; | |||
| self._inflightRequests++; | |||
There was a problem hiding this comment.
It seems that this increments the inflight requests number only for the initial request, but not for retries? On the other hand it seems we decrement number of inflight requests on raw requests' events? If that's correct it seems we'd want that behavior to be symmetrical, otherwise we'd run into cases where the number of inflight requests is < 0.
There was a problem hiding this comment.
Great catch! I thought I would catch this with the connection timeout test but it didn't because I set retries: 1 🤦♂️
|
Thanks for the feedback! Comments addressed, feel free to take another pass at your leisure. |
lib/HttpClient.js
Outdated
| var self = this; | ||
| self._inflightRequests--; | ||
| assert.ok(self._inflightRequests >= 0, | ||
| 'number of inflight requests cannot be <= 0'); |
There was a problem hiding this comment.
nit: "cannot be < 0".
test/inflightRequests.js
Outdated
| url: 'http://localhost:3000/', | ||
| requestTimeout: 100 | ||
| }); | ||
| var STRINGCLIENT = clients.createStringClient({ |
There was a problem hiding this comment.
Do we care about testing a JsonClient too?
test/inflightRequests.js
Outdated
| STRINGCLIENT.get('/200', _.noop); | ||
| assert.strictEqual(STRINGCLIENT.inflightRequests(), 4); | ||
|
|
||
| setTimeout(function () { |
There was a problem hiding this comment.
On a busy system, it could timeout before all requests and responseshave been processed, and thus could make this test flaky.
What do you think of passing the same callback to all STRINGCLIENT.get calls that would decrement a counter initialized at 4, close the server when that counter reaches 0, and then call done() in the listener for the server's 'close' event?
There was a problem hiding this comment.
Good call. I was being lazy. 👍
|
Thanks @misterdjules! Last time around, I hope? |
test/inflightRequests.js
Outdated
| url: 'http://localhost:3000/', | ||
| requestTimeout: 100 | ||
| }); | ||
| var JSONCLIENT = clients.createStringClient({ |
There was a problem hiding this comment.
Should be createJsonClient.
No description provided.