-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
oldest.destroy(); | ||
} else { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit, else if (!freeSockets) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not quite following, could you explain a bit more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the block inside of the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
@@ -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') { | ||
|
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(); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, freeSockets.shift()