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 1 commit
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
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();