Skip to content

Embrace Errors#53

Closed
eriktrom wants to merge 4 commits into
masterfrom
feature/embrace-the-errorz
Closed

Embrace Errors#53
eriktrom wants to merge 4 commits into
masterfrom
feature/embrace-the-errorz

Conversation

@eriktrom

@eriktrom eriktrom commented Mar 8, 2017

Copy link
Copy Markdown
Member

No description provided.

@eriktrom eriktrom mentioned this pull request Mar 8, 2017
Closed
ro0gr
ro0gr reviewed Mar 8, 2017
View reviewed changes
Comment thread test/port-finder-z-integration-test.js
process.kill(child.pid);
callback(null, "timeout");
}, 10000); // 10 seconds
}, 20000); // 10 seconds

@ro0gr ro0gr Mar 8, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove "10 seconds" comment?

@eriktrom eriktrom Mar 9, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @ro0gr 👍

@eriktrom

eriktrom commented Mar 9, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

Checklist

  • handle when to iterate to next host vs when to iterate to next port
    • allow 10-50 normal listen errors
    • on first client.write that says server is listening but does not pipe text through, shift off that host and iterate to next
  • Remove hiddenButImportantHost priming ???

ro0gr
ro0gr reviewed Mar 15, 2017
View reviewed changes
Comment thread lib/portfinder.js Outdated
hiddenButImportantHost = '0.0.0.0', // !important - dont remove, hence the naming :)
results = [hiddenButImportantHost];
hiddenButImportantHost = ['0.0.0.0', '::'], // !important - dont remove, hence the naming :)
results = [].concat(hiddenButImportantHost);

@ro0gr ro0gr Mar 15, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this concat? Looks like assignment should be sufficient

results = ['0.0.0.0', '::']

@ro0gr

ro0gr commented Mar 15, 2017

Copy link
Copy Markdown

I've tried it locally and have the following error

events.js:160
      throw er; // Unhandled 'error' event
      ^

Error: connect EADDRNOTAVAIL 0.0.0.0:4200
    at Object.exports._errnoException (util.js:1022:11)
    at exports._exceptionWithHostPort (util.js:1045:20)
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1087:14)

I'm on win10.
os.networkInterfaces() does not throw and returns the whole interfaces list.
Hope it helps

@eriktrom

eriktrom commented Mar 21, 2017

Copy link
Copy Markdown
Member Author

@ro0gr - hey appreciate u taking the time to test it out. Regarding the error with 0.0.0.0 - I'm considering taking that out along with '::'. Did u try that by any chance?

Either way thanks a bunch for ur feedback.

@eriktrom eriktrom mentioned this pull request May 7, 2017
Merged
@eriktrom
Merge branch 'master' of github.com:indexzero/node-portfinder into fe…
b86739d 
…ature/embrace-the-errorz

* 'master' of github.com:indexzero/node-portfinder:
  Make test close listening sockets instead of just removing them
@eriktrom

eriktrom commented Jan 11, 2018

Copy link
Copy Markdown
Member Author

closing - will re-open if such codez are needed

@eriktrom eriktrom closed this Jan 11, 2018
@eriktrom eriktrom deleted the feature/embrace-the-errorz branch February 26, 2025 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@ro0gr ro0gr ro0gr left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@eriktrom @ro0gr