Remove ipv6 from default hosts on failure

[eriktrom]

Remove ipv6 from default hosts that we test each port against
to allow portfinder to work with machines that dont have ipv6

fixes #26 #28

[eriktrom]

@jeffcharles @corpulentcoffee - do you mind pulling this branch and trying it out. Thanks!
@winfinit @indexzero - can you review for code for clarity, sanity check
@stefanpenner (if u have time, take a pass through for sanity)

[eriktrom]

If anyone has suggestions for writing a unit test, I'm all ears

Also, if anyone knows how to manually test this, I'm all ears. Can I disable ::1 on my mac? Probably. What will happen to my machine? No idea. If it crashes will it reboot with it disabled? No idea. Thus, I did not try. Will wait for someone to confirm that filed a bug here or upstream

(unless of course someone has a good stack overflow post they found saying i can manually test this without hurting my dear laptop in any way, even if it crashes before i re-enable ipv6 (poor thing crashes often actually, too often for a mac))

[eriktrom]

btw - i based the code on the idea that error.address would provide the address, and if it was ipv6, i filter out all ipv6 addresses. If this logic does not hold, aka, error.address always is provided, then we'll need another approach(but that's the cleanest). Does that logic hold? (for all OS's and node versions?)

{ [Error: listen EADDRNOTAVAIL ::1:8000]
  code: 'EADDRNOTAVAIL',
  errno: 'EADDRNOTAVAIL',
  syscall: 'listen',
  address: '::1',
  port: 8000 }

[corpulentcoffee]

@corpulentcoffee - do you mind pulling this branch and trying it out. Thanks!

FWIW, when I run with this patch, it works, but it doesn't seem to check every host in the list anymore.

e.g. If using something like require('./portfinder').getPort(function myCallback() { ... }), it looks like internals.testPort() is called for ::1 and that fails, then internals.testPort() is called for 127.0.0.1 and that succeeds, and myCallback() is called with a port... but internals.testPort() never gets called to check the port on 0.0.0.0 and the new err.address && net.isIPv6(err.address) code never gets exercised because the callback to async.everyLimit() isn't made when ::1 fails (it seems to only be called after 127.0.0.1 succeeds).

[eriktrom]

@corpulentcoffee - thank you so much - can you console.log(openPorts) right here https://github.com/indexzero/node-portfinder/blob/handle-ipv6-errors/lib/portfinder.js#L121 and tell me if there are 2 items in the array or just one - don't pass options.host, only options.port (just to clarify and isolate)

thanks again

[eriktrom]

oh i see it, https://github.com/indexzero/node-portfinder/blob/handle-ipv6-errors/lib/portfinder.js#L105-L109 is nested under options.host check - run that with an options.host and it should work, update coming now

[eriktrom]

sorry, my first answer was correct, do still mind logging that - ignore my comment about nesting under options.port, that was on purpose - the only other thing i see is that i don't use the async library so I need to look up everyLimit, 1 but @winfinit has assured me it is correct.

if you do log that, let me know

[eriktrom]

everyLimit is now called eachLimit - perhaps before it acted differently as @winfinit suggested - but as I have wondered and asked him - the limit is the number of times the iterator is called, so 127.0.0.1 is the only host tested, your correct - test for eachLimit, 0 - https://github.com/caolan/async/blob/master/test/test-async.js#L1039-L1049

anyone know which of the async methods i should use? I've got read the tests to find it, promises are my go to for a while now

[corpulentcoffee]

Hey @eriktrom:

I added that plus some other logging, here's the output:

$ node
> require('./portfinder').
... getPort(function() { console.log("done!", arguments); });
[...]
in everyLimit() iteration callback: host is ::1
entered testPort(): trying ::1 port 8000
done w/ testPort(): failed ::1 w/ port 8000 with error EAFNOSUPPORT
in everyLimit() iteration callback testPort() callback with an err: EAFNOSUPPORT
in everyLimit() iteration callback: host is 127.0.0.1
entered testPort(): trying 127.0.0.1 port 8000
done w/ testPort(): OK 127.0.0.1 port 8000
in everyLimit() iteration callback testPort() callback with a success for port 8000
in everyLimit() result callback: err is false
in everyLimit() result callback: openPorts is [ 8000 ]
done! { '0': null, '1': 8000 }

[eriktrom]

in everyLimit() iteration callback testPort() callback with a success for port 8000 <-- only called once, async.everyLimit is the issue. The rest works though, which is awesome. Will find the right method. Again, your awesome @corpulentcoffee

[eriktrom]

@corpulentcoffee - i've replaced everyLimit with async.each which looks correct from their tests - can you give one more shot and let me know your log output. Should work as expected now. Thanks again.

[corpulentcoffee]

@eriktrom This plain async.each version hangs the process for a few seconds and then returns 65536 as the "available" port for me. I think the problem now might be that you're entering testPort() at the same time with the same port but on different hosts, so at least one of them fails with EADDRINUSE.

Here's the log with just the "entered testPort()" and "done w/ testPort()" messages:

entered testPort(): trying 0.0.0.0 port 17291
done w/ testPort(): OK 0.0.0.0 port 17291
entered testPort(): trying 127.0.0.1 port 17291
entered testPort(): trying 0.0.0.0 port 17291
done w/ testPort(): OK 127.0.0.1 port 17291
done w/ testPort(): failed 0.0.0.0 w/ port 17291 with error EADDRINUSE
entered testPort(): trying 0.0.0.0 port 17292
done w/ testPort(): OK 0.0.0.0 port 17292
entered testPort(): trying 127.0.0.1 port 17292
entered testPort(): trying 0.0.0.0 port 17292
done w/ testPort(): OK 127.0.0.1 port 17292
done w/ testPort(): failed 0.0.0.0 w/ port 17292 with error EADDRINUSE

[eriktrom]

yeah - must have been concurrent - looks like eachSeries is right choice - travis and appveyor (0.12 && 4.2 so far) now pass(for the push)

[eriktrom]

I think this means it works - (merged @corpulentcoffee's logging branch then ran tests) https://gist.github.com/eriktrom/6c38c85b85b4dfa9946b

Appveyor for the pr(not the branch, aka the last push) is acting up. I rebased and re-pushed, hopefully it'll pull through, either way I think this is ready

@corpulentcoffee - things good on your end now?

EDIT: the tests that start with in everyLimit() iteration callback: host is start a child process that has a timeout so the ports should fail for those where they do

[corpulentcoffee]

@eriktrom: So far, the latest version using async.eachSeries works for me (node 4.4.0 on Linux 4.4.5 w/ the ipv6.disable=1 kernel flag)!

One very minor harmless thing: when using something like portfinder.getPort({host: '127.0.0.2'}, function() { ... }), the IPv4-only re-try attempt will scan whatever the user passed as options.host twice.

        } else {
          // filter our defaultHosts to only be ipv4, start over
          defaultHosts = defaultHosts.filter(function(_host) {
            return net.isIPv4(_host);
          });
          return exports.getPort(options, callback);  // <-- options gets re-passed here
        }

...

  if (options.host) {
    defaultHosts.push(options.host);  // <-- and then the options.host is re-pushed here
  }

Maybe check to make sure options.host isn't already in the list before pushing it onto defaultHosts?

[eriktrom]

@corpulentcoffee - again your feedback has been crucial to shipping this thanks so much

regarding checking it twice - the reason we don't filter them is b/c the same port can be checked twice like in the case where you pass localhost and it resolves to 127.0.0.1 we check twice anyway - creating a non sparse array that removes duplicates is more complexity to the codebase, at the cost of double checking only one host twice, which we might do anyway(in the localhost case) so I didn't filter out duplicates into a non sparse array. (There are issues if the array is sparse btw b/c the missing items become undefined and thus add a host of undefined which resolves to odd things on different systems IIRC)

(if you have a loop for that though, drop it here, i'll add it :) )

[eriktrom]

oh i do see, we add the same provided host for every iteration, adding up over time, haha, k

[eriktrom]

fixed - nice catch there - would have kept pushing on the same user provided host for ports that we're not open, slowing down the code over time. was more than just one extra check, thanks!

[stefanpenner]

Keep up the good work guys :)

[eriktrom]

@indexzero - push to npm

[jeffcharles]

Works for me

[stefanpenner]

👯