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

[snytkine]

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?

[targos]

@nodejs/http

[ronag]

@rickyes

[ronag]

#40452