For connection name, adjust for new node versions

[mgorczyca]

For node.js v0.12.0 and iojs-v1.5.1, host is a structure instead of a string. Also, any existing localAddress is part of the connection name.

[mblair]

This change doesn't work for me; I'm running Node v0.12.1. My host structure looks like this, when running a server locally:

{ path: undefined,
  port: 443,
  host: 'flipboard.com',
  maxSockets: 1000,
  servername: 'flipboard.com',
  encoding: null }

The following patch (on top of master) works for me, at least locally:

diff --git a/index.js b/index.js
index 417b1de..131f62a 100644
--- a/index.js
+++ b/index.js
@@ -16,13 +16,17 @@ function ForeverAgent(options) {
   self.maxSockets = self.options.maxSockets || Agent.defaultMaxSockets
   self.minSockets = self.options.minSockets || ForeverAgent.defaultMinSockets
   self.on('free', function(socket, host, port) {
-    var name = host + ':' + port
+    // For node.js v0.12.0 and iojs-v1.5.1, host is a structure instead of a string
+    // Also, any existing localAddress is part of the connection name
+    var name = typeof host === 'string' ? host + ':' + port :
+                                          host.host + ':' + host.port + (host.localAddress ? (':' + host.localAddress + ':') : '::')
+
     if (self.requests[name] && self.requests[name].length) {
       self.requests[name].shift().onSocket(socket)
     } else if (self.sockets[name].length < self.minSockets) {
       if (!self.freeSockets[name]) self.freeSockets[name] = []
       self.freeSockets[name].push(socket)
-      
+
       // if an error happens while we don't use the socket anyway, meh, throw the socket away
       var onIdleError = function() {
         socket.destroy()
@@ -53,7 +57,11 @@ ForeverAgent.prototype.addRequest = function(req, host, port) {
     host = options.host
   }

-  var name = host + ':' + port
+  // For node.js v0.12.0 and iojs-v1.5.1, host is a structure instead of a string
+  // Also, any existing localAddress is part of the connection name
+  var name = typeof host === 'string' ? host + ':' + port :
+                                        host.host + ':' + host.port + (host.localAddress ? (':' + host.localAddress + ':') : '::')
+
   if (this.freeSockets[name] && this.freeSockets[name].length > 0 && !req.useChunkedEncodingByDefault) {
     var idleSocket = this.freeSockets[name].pop()
     idleSocket.removeListener('error', idleSocket._onIdleError)

[mgorczyca]

Yes, your change works for me, too. And I can see that it is a more generalized fix. Thank you!

[mblair]

Cool, the amended diff works great!

@nylen any chance we could get this merged and released?

[nylen]

I'd rather not have more than one immediate if clause ? : on a line, especially if they are nested. @mgorczyca or @mblair can you refactor this to use if blocks? That way you can also put the comments in the right branch of the if statement where they actually apply.

[nylen]

Another reason to do this using if blocks: it doesn't help readability that there are lots of ':' strings interspersed with the : operator.

[simov]

Alternative solution would be to at least split the branches into lines

var name = typeof host === 'string'
    ? host + ':' + port
    : host.host + ':' + host.port + (host.localAddress ? (':' + host.localAddress + ':') : '::')

but if/else would do the job too, if you put var name before it

[mgorczyca]

Thanks for the feedback. I refactored the code to address the concerns, use a function to avoid duplicate code, and delete lines in addRequest that I think are no longer needed.

[mblair]

Awesome, this patch works for me!

[simov]

@mgorczyca return back

if (typeof host !== 'string') {
    var options = host
    port = options.port
    host = options.host
  }

where it was. Clearly that's not related to the code you are refactoring.

[mgorczyca]

What caught my eye about those lines is that my code for creating the connection name assumes unchanged values for port and host. I overlooked that the potentially changed values for port and host are used in addRequestNoreuse. My apologies, and thank you for pointing out the issue.

[simov]

No problem, we don't have tests for this project so things like that happen ... We kind of have that tested in request, so the tests were failing after I pulled the agent from here.

[simov]

@nylen with this fix the test in request/request#1516 passes, though it times out on server close, but that's not related to this PR (more likely related to #12)

[simov]

@nylen the test regarding this fix in request is ready https://github.com/request/request/pull/1516/files so I'd suggest a minor release version for forever-agent 0.6.1

[mblair]

Bump! This one issue is keeping us off of Node v0.12.

[simov]

Published on NPM!

[mblair]

Thank you!

[nylen]

ty @simov, catching up on emails again.