Can someone take a look at this code in _http_agent.js there may be a bug.

[ronag]

Discussed in #40443

^{Originally posted by snytkine October 13, 2021}
I'm not sure if this is the correct forum for this. Also I have never worked on node.js code itself so I may not have a complete understanding how the agent manages sockets. I was reviewing the code in _http_agent.js and I think that several things regarding counters of sockets and free sockets are incorrect.

I want someone to review this code to confirm

In _http_agent.js around line 266 in function addRequest

  let socket;
  if (freeSockets) {
    while (freeSockets.length && freeSockets[0].destroyed) {
      ArrayPrototypeShift(freeSockets);
    }
    socket = this.scheduling === 'fifo' ?
      ArrayPrototypeShift(freeSockets) :
      ArrayPrototypePop(freeSockets);
    if (!freeSockets.length)
      delete this.freeSockets[name];
  }

  const freeLen = freeSockets ? freeSockets.length : 0;
  const sockLen = freeLen + this.sockets[name].length;

  if (socket) {
    asyncResetHandle(socket);
    this.reuseSocket(socket, req);
    setRequestSocket(this, req, socket);
    ArrayPrototypePush(this.sockets[name], socket);
    this.totalSocketCount++;
  } 

I understand that if socket is found in freeSockets then this socket will be used. But what I don't understand is why this.totalSocketCount is incremented at this point?

I mean the socket was basically taken from a pool of freeSockets and as far as I understand it the socket in freeSockets is already counted in this.totalSocketCount

I think this counter should only be incremented when new socket is created and decremented when socket is destroyed. But in this case it not a new socket so counter should not be incremented.

if this is a bug then it can potentially increment totalSocketCount to a much higher number when sockets are being reused, preventing the system from creating new socket when new socket is needed

Can someone take a second look at it and confirm or deny my findings?

[ronag]

I agree it looks suspicious that totalSocketCount is increment on socket re-use.

[ronag]

@nodejs/http @rickyes

[lpinca]

It looks like a bug.

[snytkine]

Thank you for looking into that. Glad I spotted it.

[mcollina]

@snytkine would you like to send a PR?

[snytkine]

Ok, I'll do it

[snytkine]

How do you guys work on this project, I mean do I have to clone this project and push change to my own clone then make PR from my repo to this repo. (Actually not sure if github supports that). Should I create new branch first then make PR or just make PR from master to master?

[lpinca]

@snytkine see https://github.com/nodejs/node/blob/master/CONTRIBUTING.md

[snytkine]

Also do I have to put defect number in commit message? Sorry, I'm now to this project, I know every project has certain standards for making PRs.

[snytkine]

Got it. I'll follow the steps and will make a PR

[jodevsa]

looks like it's not a bug. We decrease the maxFreeSockets when we remove the socket from the list of sockets:
https://github.com/nodejs/node/blob/master/lib/_http_agent.js#L187

I was also able to verify that using the following test : #40572

I think we have other issues, like the maxFreeSockets going down to -1 ( check my comments on the PR)

[lpinca]

@jodevsa the counter should be incremented only when a socket is created and decremented only a the socket is closed or released. This seems to fix the issue:

diff --git a/lib/_http_agent.js b/lib/_http_agent.js
index a42c0e8399..87450993a6 100644
--- a/lib/_http_agent.js
+++ b/lib/_http_agent.js
@@ -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);
@@ -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);
@@ -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--;
     agent.removeSocket(s, options);
     s.removeListener('close', onClose);
     s.removeListener('free', onFree);
@@ -438,7 +439,6 @@ Agent.prototype.removeSocket = function removeSocket(s, options) {
         // Don't leak
         if (sockets[name].length === 0)
           delete sockets[name];
-        this.totalSocketCount--;
       }
     }
   }
diff --git a/test/parallel/test-http-agent-keepalive.js b/test/parallel/test-http-agent-keepalive.js
index a1f902fab0..faf605e38d 100644
--- a/test/parallel/test-http-agent-keepalive.js
+++ b/test/parallel/test-http-agent-keepalive.js
@@ -70,9 +70,11 @@ function second() {
     res.on('end', common.mustCall(() => {
       assert.strictEqual(agent.sockets[name].length, 1);
       assert.strictEqual(agent.freeSockets[name], undefined);
+      assert.strictEqual(agent.totalSocketCount, 1);
       process.nextTick(common.mustCall(() => {
         assert.strictEqual(agent.sockets[name], undefined);
         assert.strictEqual(agent.freeSockets[name].length, 1);
+        assert.strictEqual(agent.totalSocketCount, 1);
         remoteClose();
       }));
     }));
@@ -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));
       }));
@@ -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));
   }));
@@ -129,9 +135,11 @@ server.listen(0, common.mustCall(() => {
     res.on('end', common.mustCall(() => {
       assert.strictEqual(agent.sockets[name].length, 1);
       assert.strictEqual(agent.freeSockets[name], undefined);
+      assert.strictEqual(agent.totalSocketCount, 1);
       process.nextTick(common.mustCall(() => {
         assert.strictEqual(agent.sockets[name], undefined);
         assert.strictEqual(agent.freeSockets[name].length, 1);
+        assert.strictEqual(agent.totalSocketCount, 1);
         second();
       }));
     }));

[jodevsa]

@lpinca I was referring to the fact that the totalSocketCount is decremented when we remove the socket, so it's not the same bug mentioned in this issue, but we had another bug; totalSocketCount being equal to 0 when we have a free socket, and totalSocketCount being equal to -1 when a free-socket has an error.

yeah I had a similar fix, but yours is cleaner. please review: #40572

[jodevsa]

sorry @snytkine if you already had started :/ I assumed you are no longer interested as 10 days have passed. Also it's really amazing that you spotted the problem just by reading the code!