fix: sockLen was being miscalculated when removing sockets

[cixel]

Order of operations issue with + and ternary statements:

console.log(0 + '' ? 'a' : 'b') // a

The code:

freeLen + this.sockets[name] ? this.sockets[name].length : 0;

Is equivalent to:

(freeLen + this.sockets[name]) ? this.sockets[name].length : 0;

When this.sockets[name] exists, it is an array, (freeLen + this.sockets[name]) evaluates to a string, which evaluates to a string (true). When it does not, (freeLen + this.sockets[name]) evaluates to NaN (false).

Either way, freeLen is ignored, which does not seem intended.

[codecov-io]

Codecov Report

Merging #60 into master will not change coverage. The diff coverage is 100%.

@@           Coverage Diff           @@
##           master      #60   +/-   ##
=======================================
  Coverage   91.51%   91.51%           
=======================================
  Files           4        4           
  Lines         283      283           
=======================================
  Hits          259      259           
  Misses         24       24

Impacted Files	Coverage Δ
lib/_http_agent.js	89.85% <100%> (ø)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 82ff0e8...723bb23. Read the comment docs.

[tony-gutierrez]

This seems like a pretty important fix for potential bugs? Why no movement here?

[fengmk2]

can you add a test case for this fix?

[cixel]

Sorry for sleeping on this, I must've missed the notifications back in July. I took another look at this today to see how I could go about adding a test.

I'll admit I'm not very familiar with the code; I found this issue doing static analysis with an AST parser on a project dependent on this one.

I made the following change to see if I could better understand the effects this issue has:

  // [patch start]
  var freeLen = this.freeSockets[name] ? this.freeSockets[name].length : 0;
  var correctSockLen = freeLen + (this.sockets[name] ? this.sockets[name].length : 0);
  var sockLen = freeLen + this.sockets[name] ? this.sockets[name].length : 0;
  // [patch end]

  if (correctSockLen !== sockLen) {
    console.log(correctSockLen, sockLen, this.maxSockets);
  }

  if (this.requests[name] && this.requests[name].length && sockLen < this.maxSockets) {
    // ...
  }

During testing, there's never a situation where correctSocklen !== sockLen and !(correctSockLen < this.maxSockets). This is the situation my patch would correct; however, I never see that, and I'm not sure how to cause that situation.

Any suggestions would be appreciated.

[fengmk2]

@cixel I see where is the bug now:

 node
> 1 + true ? 1 : 0
1
> 1 + (true ? 1 : 0)
2

[fengmk2]

3.5.2 released

@cixel Thanks.

[cixel]

Thank you! Sorry for the delayed follow-up.