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

Discussed in https://github.com/nodejs/node/discussions/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?

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

@rickyes

@nodejs/http @rickyes

It looks like a bug.

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

@snytkine

@snytkine would you like to send a PR?

Ok, I'll do it

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?

@snytkine

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

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.

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

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)

@jodevsa

@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();
       }));
     }));

@lpinca

@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

@snytkine

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!

Navigation Menu

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

Discussed in https://github.com/nodejs/node/discussions/40443

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

Description

Discussed in https://github.com/nodejs/node/discussions/40443

Activity

ronag commented on Oct 14, 2021

ronag commented on Oct 14, 2021

lpinca commented on Oct 14, 2021

snytkine commented on Oct 14, 2021

mcollina commented on Oct 14, 2021

snytkine commented on Oct 14, 2021

snytkine commented on Oct 14, 2021

lpinca commented on Oct 14, 2021

snytkine commented on Oct 14, 2021

snytkine commented on Oct 14, 2021

jodevsa commented on Oct 23, 2021

lpinca commented on Oct 23, 2021

jodevsa commented on Oct 23, 2021

jodevsa commented on Oct 24, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions