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.

Sign up for GitHub

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

Jump to bottom

errors: provide defaults for unmapped uv errors #29288

Closed
wants to merge 1 commit into from
Closed

errors: provide defaults for unmapped uv errors #29288

wants to merge 1 commit into from

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Aug 23, 2019 •
edited
Loading

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
bnoordhuis
bnoordhuis approved these changes Aug 25, 2019
View reviewed changes
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

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

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

@cjihrig
errors: provide defaults for unmapped uv errors
8088dfb 
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 •
edited by cjihrig
Loading

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
@cjihrig cjihrig mentioned this pull request Aug 27, 2019
Closed
@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
@cjihrig @Trott
errors: provide defaults for unmapped uv errors
c246f81 
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
@cjihrig @BridgeAR
errors: provide defaults for unmapped uv errors
f56cd36 
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
Merged
BridgeAR pushed a commit that referenced this pull request Sep 4, 2019
@cjihrig @BridgeAR
errors: provide defaults for unmapped uv errors
30b80e5 
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
Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@lpinca lpinca lpinca approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cjihrig @nodejs-github-bot @Trott @bnoordhuis @lpinca