Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

For connection name, adjust for new node versions #27

Merged
merged 4 commits into from
Apr 7, 2015

Conversation

mgorczyca
Copy link
Contributor

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.

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
Copy link

mblair commented Mar 26, 2015

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
Copy link
Contributor Author

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

@mblair
Copy link

mblair commented Mar 27, 2015

Cool, the amended diff works great!

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

@nylen
Copy link
Member

nylen commented Apr 1, 2015

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
Copy link
Member

nylen commented Apr 1, 2015

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

@simov
Copy link
Member

simov commented Apr 1, 2015

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

Used an 'if' block, as requested. Created getConnectionName function to avoid duplicate code. Deleted lines no longer needed in addRequest.
@mgorczyca mgorczyca changed the title For connection name, adjust for new node vesions For connection name, adjust for new node versions Apr 1, 2015
@mgorczyca
Copy link
Contributor Author

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
Copy link

mblair commented Apr 2, 2015

Awesome, this patch works for me!

@simov
Copy link
Member

simov commented Apr 2, 2015

@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.

The fix for forming the connection name assumes unaltered port and host. However, the potentially altered port and host are used in addRequestNoreuse. So those lines are, indeed, still needed.
@mgorczyca
Copy link
Contributor Author

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
Copy link
Member

simov commented Apr 2, 2015

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
Copy link
Member

simov commented Apr 2, 2015

@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
Copy link
Member

simov commented Apr 3, 2015

@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
Copy link

mblair commented Apr 7, 2015

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

simov added a commit that referenced this pull request Apr 7, 2015
For connection name, adjust for new node versions
@simov simov merged commit 8304db3 into request:master Apr 7, 2015
@simov
Copy link
Member

simov commented Apr 7, 2015

Published on NPM!

@mblair
Copy link

mblair commented Apr 7, 2015

Thank you!

@nylen
Copy link
Member

nylen commented Apr 18, 2015

ty @simov, catching up on emails again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants