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 totalSocketCount only on socket creation or when the socket is closed #40572

Merged
merged 1 commit into from
Nov 10, 2021
Merged
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
4 changes: 2 additions & 2 deletions lib/_http_agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,6 @@ Agent.prototype.addRequest = function addRequest(req, options, port/* legacy */,
this.reuseSocket(socket, req);
setRequestSocket(this, req, socket);
ArrayPrototypePush(this.sockets[name], socket);
this.totalSocketCount++;
} else if (sockLen < this.maxSockets &&
this.totalSocketCount < this.maxTotalSockets) {
debug('call onSocket', sockLen, freeLen);
Expand Down Expand Up @@ -383,6 +382,7 @@ function installListeners(agent, s, options) {
// This is the only place where sockets get removed from the Agent.
// If you want to remove a socket from the pool, just close it.
// All socket errors end in a close event anyway.
agent.totalSocketCount--;
agent.removeSocket(s, options);
}
s.on('close', onClose);
Expand All @@ -406,6 +406,7 @@ function installListeners(agent, s, options) {
// (defined by WebSockets) where we need to remove a socket from the
// pool because it'll be locked up indefinitely
debug('CLIENT socket onRemove');
agent.totalSocketCount--;
jodevsa marked this conversation as resolved.
Show resolved Hide resolved
agent.removeSocket(s, options);
s.removeListener('close', onClose);
s.removeListener('free', onFree);
Expand Down Expand Up @@ -438,7 +439,6 @@ Agent.prototype.removeSocket = function removeSocket(s, options) {
// Don't leak
if (sockets[name].length === 0)
delete sockets[name];
this.totalSocketCount--;
}
}
}
Expand Down
7 changes: 7 additions & 0 deletions test/parallel/test-http-agent-keepalive.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ function checkDataAndSockets(body) {
assert.strictEqual(body.toString(), 'hello world');
assert.strictEqual(agent.sockets[name].length, 1);
assert.strictEqual(agent.freeSockets[name], undefined);
assert.strictEqual(agent.totalSocketCount, 1);
}

function second() {
Expand All @@ -73,6 +74,7 @@ function second() {
process.nextTick(common.mustCall(() => {
assert.strictEqual(agent.sockets[name], undefined);
assert.strictEqual(agent.freeSockets[name].length, 1);
assert.strictEqual(agent.totalSocketCount, 1);
remoteClose();
}));
}));
Expand All @@ -91,10 +93,12 @@ function remoteClose() {
process.nextTick(common.mustCall(() => {
assert.strictEqual(agent.sockets[name], undefined);
assert.strictEqual(agent.freeSockets[name].length, 1);
assert.strictEqual(agent.totalSocketCount, 1);
// Waiting remote server close the socket
setTimeout(common.mustCall(() => {
assert.strictEqual(agent.sockets[name], undefined);
assert.strictEqual(agent.freeSockets[name], undefined);
assert.strictEqual(agent.totalSocketCount, 0);
remoteError();
}), common.platformTimeout(200));
}));
Expand All @@ -110,10 +114,12 @@ function remoteError() {
assert.strictEqual(err.message, 'socket hang up');
assert.strictEqual(agent.sockets[name].length, 1);
assert.strictEqual(agent.freeSockets[name], undefined);
assert.strictEqual(agent.totalSocketCount, 1);
// Wait socket 'close' event emit
setTimeout(common.mustCall(() => {
assert.strictEqual(agent.sockets[name], undefined);
assert.strictEqual(agent.freeSockets[name], undefined);
assert.strictEqual(agent.totalSocketCount, 0);
server.close();
}), common.platformTimeout(1));
}));
Expand All @@ -132,6 +138,7 @@ server.listen(0, common.mustCall(() => {
process.nextTick(common.mustCall(() => {
assert.strictEqual(agent.sockets[name], undefined);
assert.strictEqual(agent.freeSockets[name].length, 1);
assert.strictEqual(agent.totalSocketCount, 1);
second();
}));
}));
Expand Down
8 changes: 6 additions & 2 deletions test/parallel/test-http-upgrade-agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,12 @@ server.listen(0, '127.0.0.1', common.mustCall(function() {
const req = http.request(options);
req.end();

req.on('socket', common.mustCall(function() {
assert.strictEqual(req.agent.totalSocketCount, 1);
}));

req.on('upgrade', common.mustCall(function(res, socket, upgradeHead) {
assert.strictEqual(req.agent.totalSocketCount, 0);
let recvData = upgradeHead;
socket.on('data', function(d) {
recvData += d;
Expand All @@ -71,14 +76,13 @@ server.listen(0, '127.0.0.1', common.mustCall(function() {
assert.strictEqual(recvData.toString(), 'nurtzo');
}));

console.log(res.headers);
jodevsa marked this conversation as resolved.
Show resolved Hide resolved
const expectedHeaders = { 'hello': 'world',
'connection': 'upgrade',
'upgrade': 'websocket' };
assert.deepStrictEqual(expectedHeaders, res.headers);

// Make sure this request got removed from the pool.
assert(!(name in http.globalAgent.sockets));
assert(!(name in req.agent.sockets));

req.on('close', common.mustCall(function() {
socket.end();
Expand Down
1 change: 0 additions & 1 deletion test/parallel/test-http-upgrade-client.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ server.listen(0, common.mustCall(function() {
assert.strictEqual(recvData.toString(), expectedRecvData);
}));

console.log(res.headers);
const expectedHeaders = {
hello: 'world',
connection: 'upgrade',
Expand Down