lib: http server, friendly error messages by sagitsofan · Pull Request #22995 · nodejs/node

sagitsofan · 2018-09-21T13:29:13Z

lib: improved error message description for the http server binding errors.

currently changed only in setupListenHandle function, but needs to be change all over.

Added new uvExceptionWithHostPort function (+export) in lib/internal/error.js that extracts the error message defined by libuv (using the error code) and returns an error object with the full error description.

example:
old error message: listen EADDRINUSE
new error message: listen EADDRINUSE: Address already in use

adjust tests to the new error.

Issue: #22936

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

lib/internal/errors.js

joyeecheung · 2018-10-05T10:19:18Z

cc @nodejs/tsc This is semver-major because it touches errors that are not in our ERR_CODE system and are potentially parsed by users.

joyeecheung · 2018-10-05T10:19:56Z

CI: https://ci.nodejs.org/job/node-test-pull-request/17646/

sagitsofan · 2018-10-05T10:28:10Z

It touches only in the error description and not in the error code which is stays the same, users are parsing the codes and not the error message.

Trott · 2018-10-05T18:50:09Z

/ping @nodejs/chakracore Is this compatible with ChakraCore or will this be something you'll need to patch over in node-chakracore if it lands in node?

Trott · 2018-10-05T18:52:26Z

users are parsing the codes and not the string.

Unfortunately, parsing of error message strings has been widespread in the ecosystem because there was no other alternative for a long time.

addaleax · 2018-10-05T18:55:10Z

users are parsing the codes and not the string.

Unfortunately, parsing of error message strings has been widespread in the ecosystem because there was no other alternative for a long time.

As far as I can tell, these errors already do have a code property, so we shouldn’t have to consider thise semver-major.

lib/internal/errors.js

jasnell · 2018-10-05T18:56:48Z

If the errors already have a code, then changes do not have to be semver-major

addaleax · 2018-10-05T19:09:18Z

Resume CI: https://ci.nodejs.org/job/node-test-pull-request/17650/

sagitsofan · 2018-10-05T19:10:10Z

Indeed errors have a code property, so users needs to parsed only the code and not the error message.
This PR changed only the description message that is returning to user.

Current error properties:

  code
  errno
  syscall
  address
  port

Trott · 2018-10-05T20:32:39Z

As far as I can tell, these errors already do have a code property, so we shouldn’t have to consider thise semver-major.

@addaleax Ah. I didn't see the usual tell-tale [ERR_CODE_THING] in the message string, but yes, indeed, there is a code property.

joyeecheung · 2018-10-05T22:17:55Z

As far as I can tell, these errors already do have a code property, so we shouldn’t have to consider thise semver-major

Yes, but these are libuv error codes, not our supposed-to-be-permanent error codes, so less justification to not go semver-major. That said we can try a CITGM and see, worse case secnario this does break and we can always revert.

addaleax · 2018-10-05T23:32:03Z

Yes, but these are libuv error codes, not our supposed-to-be-permanent error codes, so less justification to not go semver-major.

For the most part, libuv forwards errors returned from the OS, which are typically spec’d and therefore usually provide more long-term stability than our errors. Plus, changing an error code in libuv would definitely be semver-major on their side, so it would also land in a semver-major change in Node.js.

That said we can try a CITGM and see, worse case secnario this does break and we can always revert.

Yup – CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1570/

mcollina

LGTM as semver-major.

lib/internal/errors.js

addaleax · 2018-10-11T05:19:15Z

@sagitsofan I don’t know why, but for some reason, there’s over 50 commits listed in this PR – could you rebase this against master?

[Super-short how-to: git fetch git@github.com:nodejs/node.git +refs/heads/master && git rebase FETCH_HEAD, possibly followed by resolving conflicts + git rebase --continue (possibly repeated), then git push --force-with-lease]

Use backticks around `SHASUM256.txt` etc. in README.md. PR-URL: nodejs#23299 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Use fast-tracking on PRs where new Collaborators are adding themselves to the README. PR-URL: nodejs#23300 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

The internal `assert` modules `errorCache` property is exposed only for testing. The one test that used it is rewritten here to not use it. This has the following advantages: * The test now makes sure that there is an empty cache in a more robust way. Instead of relying on the internal implementation of `errorCache`, it simply spawns a separate process. * One less test using the `--expose-internals` flag. PR-URL: nodejs#23304 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

The internal assert module exposed an errorCache property solely for testing. It is no longer necessary. Remove it. PR-URL: nodejs#23304 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

Comments clarification at the deprecated function and new function. Fixes: nodejs#22936

Comments clarification Fixes: nodejs#22936

sagitsofan · 2018-10-12T02:10:02Z

@addaleax Done, can it land?

addaleax · 2018-10-12T02:29:10Z

CI: https://ci.nodejs.org/job/node-test-pull-request/17746/

@sagitsofan Thanks! There should be nothing you need to do here. :) I’ve added the author ready label, which means we’ll land this as soon as we can. If that doesn’t happen (and CI is good), feel free to ping us about that.

addaleax · 2018-10-12T04:25:34Z

Landed in 82ea705 🎉 Thanks for the PR!

Improved error message description for the http server binding errors. Currently changed only in `setupListenHandle`, but needs to be change all over. Added new `uvExceptionWithHostPort` function (+export) in `lib/internal/error.js` that extracts the error message defined by libuv, using the error code, and returns an error object with the full error description. example: old error message: `listen EADDRINUSE` new error message: `listen EADDRINUSE: Address already in use` Removed exportable function `_exceptionWithHostPort` from `lib/util.js` - exported by accident Replaced `exceptionWithHostPort` to the new function `uvExceptionWithHostPort` for a more detailed error. Fixes: #22936 PR-URL: #22995 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

ericandrewlewis · 2018-10-12T16:58:33Z

🎊 this will come out in version 12, right? Since the semver-major cut-off date for version 11 was October 2nd.

Improved error message description for the http server binding errors. Currently changed only in `setupListenHandle`, but needs to be change all over. Added new `uvExceptionWithHostPort` function (+export) in `lib/internal/error.js` that extracts the error message defined by libuv, using the error code, and returns an error object with the full error description. example: old error message: `listen EADDRINUSE` new error message: `listen EADDRINUSE: Address already in use` Removed exportable function `_exceptionWithHostPort` from `lib/util.js` - exported by accident Replaced `exceptionWithHostPort` to the new function `uvExceptionWithHostPort` for a more detailed error. Fixes: #22936 PR-URL: #22995 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

sagitsofan · 2018-10-13T15:59:12Z

@ericandrewlewis Version 10 is at October not version 11,
see https://github.com/nodejs/Release#release-schedule

Improved error message description for the http server binding errors. Currently changed only in `setupListenHandle`, but needs to be change all over. Added new `uvExceptionWithHostPort` function (+export) in `lib/internal/error.js` that extracts the error message defined by libuv, using the error code, and returns an error object with the full error description. example: old error message: `listen EADDRINUSE` new error message: `listen EADDRINUSE: Address already in use` Removed exportable function `_exceptionWithHostPort` from `lib/util.js` - exported by accident Replaced `exceptionWithHostPort` to the new function `uvExceptionWithHostPort` for a more detailed error. Fixes: #22936 PR-URL: #22995 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. net Issues and PRs related to the net subsystem. labels Sep 21, 2018

mscdex reviewed Sep 21, 2018

View reviewed changes

lib/internal/errors.js Outdated Show resolved Hide resolved

mscdex reviewed Sep 21, 2018

View reviewed changes

lib/internal/errors.js Outdated Show resolved Hide resolved

sagitsofan mentioned this pull request Sep 21, 2018

Improve error message for address already in use #22936

Closed

sagitsofan changed the title ~~Error msg 3~~ Https server :: Friendly error messages Sep 21, 2018

sagitsofan changed the title ~~Https server :: Friendly error messages~~ http server : friendly error messages Sep 21, 2018

sagitsofan changed the title ~~http server : friendly error messages~~ lib: http server, friendly error messages Sep 21, 2018

joyeecheung added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 5, 2018

addaleax reviewed Oct 5, 2018

View reviewed changes

lib/internal/errors.js Outdated Show resolved Hide resolved

addaleax approved these changes Oct 5, 2018

View reviewed changes

addaleax removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 5, 2018

mcollina approved these changes Oct 6, 2018

View reviewed changes

thefourtheye approved these changes Oct 6, 2018

View reviewed changes

lib/internal/errors.js Outdated Show resolved Hide resolved

lib/internal/errors.js Outdated Show resolved Hide resolved

sagitsofan force-pushed the error-msg-3 branch 2 times, most recently from e1d2978 to c3738fb Compare October 10, 2018 20:01

sagitsofan force-pushed the error-msg-3 branch from cd55d21 to e0cba9c Compare October 11, 2018 18:27

Trott and others added 8 commits October 12, 2018 04:40

doc: use backticks around file names in README.md

42dd59d

Use backticks around `SHASUM256.txt` etc. in README.md. PR-URL: nodejs#23299 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

doc: update onboarding task

7a3b34a

Use fast-tracking on PRs where new Collaborators are adding themselves to the README. PR-URL: nodejs#23300 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

lib: changed function comment

c1cf5c6

Comments clarification at the deprecated function and new function. Fixes: nodejs#22936

lib: changed function comment

5804965

Comments clarification Fixes: nodejs#22936

fix conflicts

9f038a5

revert changes

fdb7b3e

sagitsofan force-pushed the error-msg-3 branch from f1d4cc0 to fdb7b3e Compare October 12, 2018 01:41

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 12, 2018

addaleax added the dont-land-on-v8.x label Oct 12, 2018

addaleax closed this Oct 12, 2018

sagitsofan deleted the error-msg-3 branch October 13, 2018 15:55

codebytere mentioned this pull request Nov 27, 2018

v10.13.1 proposal #24675

Closed

codebytere mentioned this pull request Nov 29, 2018

v10.14.2 proposal #24727

Merged

This was referenced Apr 13, 2019

http.get returns "getaddrinfo ENOTFOUND" error #5436

Closed

dns: provide additional info for ENOTFOUND errors #26903

Closed

Uh oh!

Conversation

sagitsofan commented Sep 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

Uh oh!

joyeecheung commented Oct 5, 2018

Uh oh!

joyeecheung commented Oct 5, 2018

Uh oh!

sagitsofan commented Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Oct 5, 2018

Uh oh!

Trott commented Oct 5, 2018

Uh oh!

addaleax commented Oct 5, 2018

Uh oh!

Uh oh!

jasnell commented Oct 5, 2018

Uh oh!

addaleax commented Oct 5, 2018

Uh oh!

sagitsofan commented Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Oct 5, 2018

Uh oh!

joyeecheung commented Oct 5, 2018

Uh oh!

addaleax commented Oct 5, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

addaleax commented Oct 11, 2018

Uh oh!

sagitsofan commented Oct 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Oct 12, 2018

Uh oh!

addaleax commented Oct 12, 2018

Uh oh!

ericandrewlewis commented Oct 12, 2018

Uh oh!

sagitsofan commented Oct 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

sagitsofan commented Sep 21, 2018 •

edited

Loading

sagitsofan commented Oct 5, 2018 •

edited

Loading

sagitsofan commented Oct 5, 2018 •

edited

Loading

sagitsofan commented Oct 12, 2018 •

edited

Loading

sagitsofan commented Oct 13, 2018 •

edited

Loading