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

http: added scheduling option to http agent #33278

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions doc/api/http.md
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,11 @@ http.get({
### `new Agent([options])`
<!-- YAML
added: v0.3.4
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/33278
description: Add `scheduling` option to specify the free socket
scheduling strategy.
-->

* `options` {Object} Set of configurable options to set on the agent.
Expand All @@ -132,6 +137,18 @@ added: v0.3.4
* `maxFreeSockets` {number} Maximum number of sockets to leave open
in a free state. Only relevant if `keepAlive` is set to `true`.
**Default:** `256`.
* `scheduling` {string} Scheduling strategy to apply when picking
the next free socket to use. It can be `'fifo'` or `'lifo'`.
The main difference between the two scheduling strategies is that `'lifo'`
selects the most recently used socket, while `'fifo'` selects
the least recently used socket.
In case of a low rate of request per second, the `'lifo'` scheduling
will lower the risk of picking a socket that might have been closed
by the server due to inactivity.
In case of a high rate of request per second,
the `'fifo'` scheduling will maximize the number of open sockets,
while the `'lifo'` scheduling will keep it as low as possible.
**Default:** `'fifo'`.
* `timeout` {number} Socket timeout in milliseconds.
This will set the timeout when the socket is created.

Expand Down
10 changes: 9 additions & 1 deletion lib/_http_agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ const { async_id_symbol } = require('internal/async_hooks').symbols;
const {
codes: {
ERR_INVALID_ARG_TYPE,
ERR_INVALID_OPT_VALUE,
},
} = require('internal/errors');
const { once } = require('internal/util');
Expand Down Expand Up @@ -86,6 +87,11 @@ function Agent(options) {
this.keepAlive = this.options.keepAlive || false;
this.maxSockets = this.options.maxSockets || Agent.defaultMaxSockets;
this.maxFreeSockets = this.options.maxFreeSockets || 256;
this.scheduling = this.options.scheduling || 'fifo';

if (this.scheduling !== 'fifo' && this.scheduling !== 'lifo') {
throw new ERR_INVALID_OPT_VALUE('scheduling', this.scheduling);
}

this.on('free', (socket, options) => {
const name = this.getName(options);
Expand Down Expand Up @@ -219,7 +225,9 @@ Agent.prototype.addRequest = function addRequest(req, options, port/* legacy */,
while (freeSockets.length && freeSockets[0].destroyed) {
freeSockets.shift();
}
socket = freeSockets.shift();
socket = this.scheduling === 'fifo' ?
freeSockets.shift() :
freeSockets.pop();
if (!freeSockets.length)
delete this.freeSockets[name];
}
Expand Down
148 changes: 148 additions & 0 deletions test/parallel/test-http-agent-scheduling.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const http = require('http');

function createServer(count) {
return http.createServer(common.mustCallAtLeast((req, res) => {
// Return the remote port number used for this connection.
res.end(req.socket.remotePort.toString(10));
}), count);
}

function makeRequest(url, agent, callback) {
http
.request(url, { agent }, (res) => {
let data = '';
res.setEncoding('ascii');
res.on('data', (c) => {
data += c;
});
res.on('end', () => {
process.nextTick(callback, data);
});
})
.end();
}

function bulkRequest(url, agent, done) {
const ports = [];
let count = agent.maxSockets;

for (let i = 0; i < agent.maxSockets; i++) {
makeRequest(url, agent, callback);
}

function callback(port) {
count -= 1;
ports.push(port);
if (count === 0) {
done(ports);
}
}
}

function defaultTest() {
const server = createServer(8);
server.listen(0, onListen);

function onListen() {
const url = `http://localhost:${server.address().port}`;
const agent = new http.Agent({
keepAlive: true,
maxSockets: 5
});

bulkRequest(url, agent, (ports) => {
Copy link
Member

Choose a reason for hiding this comment

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

Might be a bit nicer to convert these into async functions with await syntax and to wrap this stack into a helper function so it can be reused in both tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for the suggestion! Unfortunately, I don't have time for refactoring this test at the moment, is it ok if we leave it as is?

Copy link
Member

Choose a reason for hiding this comment

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

@jasnell is not clear if you have an hard block on the tests being converted or not. Can we land this anyway?

Copy link
Member

Choose a reason for hiding this comment

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

No hard block!

makeRequest(url, agent, (port) => {
assert.strictEqual(ports[0], port);
makeRequest(url, agent, (port) => {
assert.strictEqual(ports[1], port);
makeRequest(url, agent, (port) => {
assert.strictEqual(ports[2], port);
server.close();
agent.destroy();
});
});
});
});
}
}

function fifoTest() {
const server = createServer(8);
server.listen(0, onListen);

function onListen() {
const url = `http://localhost:${server.address().port}`;
const agent = new http.Agent({
keepAlive: true,
maxSockets: 5,
scheduling: 'fifo'
});

bulkRequest(url, agent, (ports) => {
makeRequest(url, agent, (port) => {
assert.strictEqual(ports[0], port);
makeRequest(url, agent, (port) => {
assert.strictEqual(ports[1], port);
makeRequest(url, agent, (port) => {
assert.strictEqual(ports[2], port);
server.close();
agent.destroy();
});
});
});
});
}
}

function lifoTest() {
const server = createServer(8);
server.listen(0, onListen);

function onListen() {
const url = `http://localhost:${server.address().port}`;
const agent = new http.Agent({
keepAlive: true,
maxSockets: 5,
scheduling: 'lifo'
});

bulkRequest(url, agent, (ports) => {
makeRequest(url, agent, (port) => {
assert.strictEqual(ports[ports.length - 1], port);
makeRequest(url, agent, (port) => {
assert.strictEqual(ports[ports.length - 1], port);
makeRequest(url, agent, (port) => {
assert.strictEqual(ports[ports.length - 1], port);
server.close();
agent.destroy();
});
});
});
});
}
}

function badSchedulingOptionTest() {
try {
new http.Agent({
keepAlive: true,
maxSockets: 5,
scheduling: 'filo'
});
} catch (err) {
assert.strictEqual(err.code, 'ERR_INVALID_OPT_VALUE');
assert.strictEqual(
err.message,
'The value "filo" is invalid for option "scheduling"'
);
}
}

defaultTest();
fifoTest();
lifoTest();
badSchedulingOptionTest();