Skip to content

Conversation

@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Aug 23, 2019

libuv does not map 100% of errors. When an unmapped error is encountered, the Map returns undefined, which is then unsuccessfully destructured, causing an exception. This commit adds a default value in the event of an unmapped error.

A test will be added once #29285 lands.

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

@nodejs-github-bot nodejs-github-bot added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Aug 23, 2019
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM but note there's another lazyErrmapGet() in lib/internal/util.js because of course there is.

That one handles missing entries by returning Unknown system error ${err}. That's arguably a little nicer for debugging.

@nodejs-github-bot
Copy link
Collaborator

@cjihrig
Copy link
Contributor Author

cjihrig commented Aug 27, 2019

LGTM but note there's another lazyErrmapGet() in lib/internal/util.js because of course there is.

That one is gone, as of #29285.

That one handles missing entries by returning Unknown system error ${err}. That's arguably a little nicer for debugging.

I've updated the PR so that uvException() and uvExceptionWithHostPort() in lib/internal/errors.js now handle undefined being returned from uvErrmapGet(). So, getSystemErrorName() in lib/internal/util.js won't be impacted by this change at all.

libuv does not map 100% of errors. When an unmapped error is
encountered, the Map returns undefined, which is then
unsuccessfully destructured, causing an exception. This commit
adds a default value in the event of an unmapped error.
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 27, 2019

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

EDIT: CI was yellow.

@cjihrig cjihrig added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 27, 2019
@Trott
Copy link
Member

Trott commented Aug 27, 2019

Landed in c246f81

@Trott Trott closed this Aug 27, 2019
Trott pushed a commit that referenced this pull request Aug 27, 2019
libuv does not map 100% of errors. When an unmapped error is
encountered, the Map returns undefined, which is then
unsuccessfully destructured, causing an exception. This commit
adds a default value in the event of an unmapped error.

PR-URL: #29288
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@cjihrig cjihrig deleted the unmapped branch August 27, 2019 18:32
BridgeAR pushed a commit that referenced this pull request Sep 3, 2019
libuv does not map 100% of errors. When an unmapped error is
encountered, the Map returns undefined, which is then
unsuccessfully destructured, causing an exception. This commit
adds a default value in the event of an unmapped error.

PR-URL: #29288
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@BridgeAR BridgeAR mentioned this pull request Sep 3, 2019
BridgeAR pushed a commit that referenced this pull request Sep 4, 2019
libuv does not map 100% of errors. When an unmapped error is
encountered, the Map returns undefined, which is then
unsuccessfully destructured, causing an exception. This commit
adds a default value in the event of an unmapped error.

PR-URL: #29288
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. errors Issues and PRs related to JavaScript errors originated in Node.js core.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants