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: Change free sockets behavior to LIFO from FIFO. #31526

Closed
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: 11 additions & 6 deletions lib/_http_agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,11 +112,16 @@ function Agent(options) {
if (this.sockets[name])
count += this.sockets[name].length;

if (count > this.maxSockets || freeLen >= this.maxFreeSockets) {
socket.destroy();
} else if (this.keepSocketAlive(socket)) {
freeSockets = freeSockets || [];
this.freeSockets[name] = freeSockets;
if (this.keepSocketAlive(socket) &&
this.maxFreeSockets > 0 &&
count <= this.maxSockets) {
if (freeLen >= this.maxFreeSockets) {
const oldest = this.freeSockets[name].shift();
Copy link
Member

Choose a reason for hiding this comment

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

nit, freeSockets.shift()

oldest.destroy();
} else {
Copy link
Member

Choose a reason for hiding this comment

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

nit, else if (!freeSockets)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not quite following, could you explain a bit more.

Copy link
Member

Choose a reason for hiding this comment

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

the block inside of the else is for the case when !freeSockets, could be simplified

Copy link
Member

Choose a reason for hiding this comment

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

it's a nit, no biggie

freeSockets = freeSockets || [];
this.freeSockets[name] = freeSockets;
}
socket[async_id_symbol] = -1;
socket._httpMessage = null;
this.removeSocket(socket, options);
Expand Down Expand Up @@ -207,7 +212,7 @@ Agent.prototype.addRequest = function addRequest(req, options, port/* legacy */,

if (freeLen) {
// We have a free socket, so use that.
const socket = this.freeSockets[name].shift();
const socket = this.freeSockets[name].pop();
// Guard against an uninitialized or user supplied Socket.
const handle = socket._handle;
if (handle && typeof handle.asyncReset === 'function') {
Expand Down
95 changes: 95 additions & 0 deletions test/sequential/test-http-keepalive-lifo-sockets.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const http = require('http');

// To test the LIFO behavior of the agent:
// Send a request and store the local TCP port
// number used to send a request to the server.

// Store a list of port numbers used to process requests...
let port_list = [];

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

server.listen(0, function() {
const agent = http.Agent({
keepAlive: true,
maxSockets: 4
});

// Fill the agent with connections
const fill_request_count = agent.maxSockets;

makeReqs(fill_request_count, function(er) {
assert.ifError(er);
// Assert that different ports were used.
assert.strictEqual(port_list.length, fill_request_count);

// Now the agent is fully populated with connections to the server
// just waiting to be reused.

// For LIFO behavior the last used port should always be used
// to send a serial string of requests.

// If there was FIFO behavior for each serial request a
// the oldest used port should be used to send the request.

const fill_ports = port_list;
port_list = [];
makeReq(common.mustCall(() => {
// Since the agent's free socket list isn't updated
// until after the response is processed. Send the next
// request completely after the first one is finished processing.
process.nextTick(common.mustCall(() => {
makeReq(common.mustCall(() => {
// LIFO should always use the last used port to send the next
// request, so verify that behavior.
const lastPort = fill_ports[fill_ports.length - 1];
assert.deepStrictEqual(port_list, [lastPort, lastPort]);
agent.destroy();
server.close();
}));
}));
}));
});

process.on('exit', function() {
console.log('ok');
});

function makeReqs(n, cb) {
for (let i = 0; i < n; i++) makeReq(then);

function then(er) {
if (er) return cb(er);
else if (--n === 0) process.nextTick(cb);
}
}

function makeReq(cb) {
http
.request(
{
port: server.address().port,
path: '/',
agent: agent
},
function(res) {
let data = '';
res.setEncoding('ascii');
res.on('data', function(c) {
data += c;
});
res.on('end', function() {
port_list.push(data);
cb();
});
}
)
.end();
}
});