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

new: add inflightRequests() API #181

Merged
merged 5 commits into from Jul 13, 2018
Merged
Show file tree
Hide file tree
Changes from 3 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
21 changes: 21 additions & 0 deletions README.md
Expand Up @@ -218,6 +218,7 @@ client.del('/foo/bar', function(err, req, res) {
});
```


### StringClient

`StringClient` is what `JsonClient` is built on, and provides a base
Expand Down Expand Up @@ -470,6 +471,26 @@ client.get(options, function(err, res, socket, head) {
});
```

### Common APIs

The following methods are shared by all versions of the client:

#### inflightRequests()

Returns the number of inflight requests the client is currently handling. The
request count is incremented as soon as a verb method is called, which means
that an inflight request is defined as a request that is in any of the
following states:

* after the verb function is called but before any I/O
* waiting on I/O (dns resolution, socket connection, server response, etc.)
* request serialization (uploading of req bodies, etc.)
* response marshalling/consumption

The counter is decremented when the response's `end` event is emitted, or when
the request's `error` event is emitted.


### Events

The client emits the following events:
Expand Down
51 changes: 50 additions & 1 deletion lib/HttpClient.js
Expand Up @@ -119,6 +119,9 @@ function rawRequest(opts, cb) {
// explicit.
var cbCalled = false;

// increment the number of currently inflight requests
opts.client._incrementInflightRequests();

/**
* this function is called after the request lifecycle has been "completed"
* and the after event is ready to be fired. this requires the consumer to
Expand Down Expand Up @@ -268,8 +271,8 @@ function rawRequest(opts, cb) {
req.getMetrics = function getMetrics() {
return metrics;
};
opts.client._decrementInflightRequests();
opts.client.emit('metrics', metrics);

emitAfter(req, res, err);
});

Expand Down Expand Up @@ -310,6 +313,7 @@ function rawRequest(opts, cb) {
log.trace({ err: realErr }, 'Request failed');
clearTimeout(connectionTimer);
clearTimeout(requestTimer);
opts.client._decrementInflightRequests();

// the user provided callback is invoked as soon as a connection is
// established. however, the request can be aborted or can fail after
Expand Down Expand Up @@ -512,6 +516,10 @@ function HttpClient(options) {

var self = this;

// internal only properties
this._inflightRequests = 0;

// options properties
this.agent = options.agent;
this.appendPath = options.appendPath || false;
this.ca = options.ca;
Expand Down Expand Up @@ -947,3 +955,44 @@ HttpClient.prototype._options = function (method, options) {

return (opts);
};


/**
* return number of currently inflight requests
* @public
* @method inflightRequests
* @returns {Number}
*/
HttpClient.prototype.inflightRequests = function inflightRequests() {
var self = this;
return self._inflightRequests;
};


/**
* increment the number of currently inflight requests
* @private
* @method inflightRequests
* @returns {Number}
*/
HttpClient.prototype._incrementInflightRequests =
function _incrementInflightRequests() {
var self = this;
self._inflightRequests++;
};


/**
* decrement the number of currently inflight requests. also make sure we never
* drop below 0, which would mean there's a bug in the way we're counting.
* @public
* @method inflightRequests
* @returns {Number}
*/
HttpClient.prototype._decrementInflightRequests =
function _decrementInflightRequests() {
var self = this;
self._inflightRequests--;
assert.ok(self._inflightRequests >= 0,
'number of inflight requests cannot be <= 0');
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "cannot be < 0".

};
205 changes: 205 additions & 0 deletions test/inflightRequests.js
@@ -0,0 +1,205 @@
'use strict';

// external files
var _ = require('lodash');
var assert = require('chai').assert;
var bunyan = require('bunyan');
var restify = require('restify');

// local files
var clients = require('../lib');


describe('inflightRequests', function () {

var SERVER;
var HTTPCLIENT = clients.createHttpClient({
url: 'http://localhost:3000/',
requestTimeout: 100
});
var STRINGCLIENT = clients.createStringClient({
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we care about testing a JsonClient too?

url: 'http://localhost:3000/',
requestTimeout: 100
});
var LOG = bunyan.createLogger({
name: 'clientlog'
});

beforeEach(function (done) {
assert.strictEqual(HTTPCLIENT.inflightRequests(), 0);
assert.strictEqual(STRINGCLIENT.inflightRequests(), 0);

SERVER = restify.createServer({
name: 'unittest',
log: LOG
});
SERVER.get('/200', function (req, res, next) {
res.send(200, { hello: 'world' });
return next();
});
SERVER.get('/500', function (req, res, next) {
res.send(500, { empty: 'world' });
return next();
});
SERVER.get('/timeout', function (req, res, next) {
setTimeout(function () {
return next();
}, 1000);
});
SERVER.use(restify.plugins.queryParser());
SERVER.listen(3000, done);
});

afterEach(function (done) {
assert.strictEqual(HTTPCLIENT.inflightRequests(), 0);
assert.strictEqual(STRINGCLIENT.inflightRequests(), 0);

HTTPCLIENT.close();
STRINGCLIENT.close();
SERVER.close(done);
});

it('StringClient should increment and decrement inflight requests',
function (done) {
// request count decremented right before callback is fired.
STRINGCLIENT.get('/200', function (err, req, res, data) {
assert.ifError(err);
assert.strictEqual(STRINGCLIENT.inflightRequests(), 0);
return done();
});

// after firing one request
assert.strictEqual(STRINGCLIENT.inflightRequests(), 1);
});

it('StringClient should increment and decrement inflight on connection ' +
'timeout',
function (done) {
// setup client to point to unresolvable IP
var client = clients.createStringClient({
url: 'http://10.255.255.1/',
connectTimeout: 100,
retry: {
minTimeout: 100,
maxTimeout: 500,
// ensure even with retries we do correct counting
retries: 3
}
});
client.get('/foo', function (err, req, res, data) {
assert.strictEqual(err.name, 'ConnectTimeoutError');
assert.strictEqual(STRINGCLIENT.inflightRequests(), 0);
return done();
});
assert.strictEqual(client.inflightRequests(), 1);
});

it('StringClient should increment and decrement inflight on request ' +
'timeout', function (done) {
// setup client to point to unresolvable IP
STRINGCLIENT.get('/timeout', function (err, req, res, data) {
assert.strictEqual(err.name, 'RequestTimeoutError');
assert.strictEqual(STRINGCLIENT.inflightRequests(), 0);
return done();
});
assert.strictEqual(STRINGCLIENT.inflightRequests(), 1);
});

it('HttpClient should increment and decrement inflight requests',
function (done) {
HTTPCLIENT.get('/200', function (err, req) {
assert.ifError(err);
assert.strictEqual(HTTPCLIENT.inflightRequests(), 1);

req.on('result', function (_err, res) {
assert.ifError(_err);
res.on('data', _.noop);
res.on('end', function () {
assert.strictEqual(HTTPCLIENT.inflightRequests(), 0);
return done();
});
});
});
});

it('HttpClient should increment and decrement connect timeout',
function (done) {
// setup client to point to unresolvable IP
var client = clients.createHttpClient({
url: 'http://10.255.255.1/',
connectTimeout: 100,
retry: false
});

client.get('/timeout', function (err, req) {
assert.ok(err);
assert.strictEqual(err.name, 'ConnectTimeoutError');
assert.strictEqual(client.inflightRequests(), 0);
return done();
});

assert.strictEqual(client.inflightRequests(), 1);
});

it('HttpClient should increment and decrement request timeout',
function (done) {
HTTPCLIENT.get('/timeout', function (err, req) {
// no connect timeout
assert.ifError(err);
assert.strictEqual(HTTPCLIENT.inflightRequests(), 1);

req.on('result', function (_err, res) {
assert.ok(_err);
assert.strictEqual(_err.name, 'RequestTimeoutError');
assert.notOk(res);
assert.strictEqual(HTTPCLIENT.inflightRequests(), 0);
return done();
});
});
});

it('HttpClient should increment and decrement on forced req abort',
function (done) {

var client = clients.createHttpClient({
url: 'http://localhost:3000'
});

client.get('/timeout', function (err, req) {
// no connect timeout
assert.ifError(err);
assert.strictEqual(client.inflightRequests(), 1);

req.on('result', function (_err, res) {
assert.strictEqual(client.inflightRequests(), 0);
assert.ok(_err);
assert.ok(_err.message, 'socket hang up');
assert.isNull(res);
return done();
});

req.abort();
});
});

it('should count multiple inflight requests', function (done) {

STRINGCLIENT.get('/200', _.noop);
assert.strictEqual(STRINGCLIENT.inflightRequests(), 1);

STRINGCLIENT.get('/200', _.noop);
assert.strictEqual(STRINGCLIENT.inflightRequests(), 2);

STRINGCLIENT.get('/200', _.noop);
assert.strictEqual(STRINGCLIENT.inflightRequests(), 3);

STRINGCLIENT.get('/200', _.noop);
assert.strictEqual(STRINGCLIENT.inflightRequests(), 4);

setTimeout(function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call. I was being lazy. 👍

// wait for all requests to complete
assert.strictEqual(STRINGCLIENT.inflightRequests(), 0);
return done();
}, 500);
});
});