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.

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

Fix error enumerablility #19719

Closed
wants to merge 1 commit into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Apr 1, 2018

  1. Commit:

A error message should always be non-enumerable. This makes sure
that is true for dns errors as well. It also adds another check
in common.expectsError to make sure no other regressions are
introduced going forward.

Fixes: #19716

2. Commit:

Right now the hostname could in some cases be missed, depending on
the libuv error number. This makes sure there the hostname is always
added, if available.

Update: I removed the second commit to another PR so it is easier for backporting.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • 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 Apr 1, 2018
@BridgeAR
Copy link
Member Author

BridgeAR commented Apr 1, 2018

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this! I think I've caught it in 67b5985 for uvException but missed the other helpers...sorry about that.

BTW there seems to be a case in lib/dns.js where the hostname is not passed into dnsException

@joyeecheung
Copy link
Member

BTW I think I previously kept the hostname from the error messages in the case of libuv errors because it changes the error messages, which is semver-major. If we keep the changes, do we need to make this semver-major? Or should we backport that in a semver-patch manner?

@BridgeAR
Copy link
Member Author

BridgeAR commented Apr 1, 2018

@joyeecheung I was wondering about the semverness of this as well. I personally would argue this is a patch because the hostname was applied in case the libuv error code would have been one of the special handled ones (that are relatively common) but not for any other ones. Of course also a bugfix can be semver-major, so I would like to defer the decision about the semverness to others. If we declare it as being semver-major, I am also happy to backport the first commit only.

@joyeecheung joyeecheung added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 1, 2018
@BridgeAR
Copy link
Member Author

BridgeAR commented Apr 1, 2018

I added another commit to always add the hostname, if available as suggested by @joyeecheung.

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

@alexjeffburke
Copy link
Contributor

alexjeffburke commented Apr 1, 2018

I think it would be better to avoid the hostname if it's going to make this change semver major. Bear in mind, this behavioural change was originally introduced in a minor version (9.7.0) so I think it would be beneficial to patch just that difference as a minor and move the hostname thing into a separate patch.

@BridgeAR BridgeAR force-pushed the fix-error-enumerablility branch 2 times, most recently from 33c5f5e to e6af409 Compare April 2, 2018 16:06
@BridgeAR BridgeAR removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 2, 2018
@BridgeAR
Copy link
Member Author

BridgeAR commented Apr 2, 2018

I updated the PR to only fix the bug and moved the second commit to another PR so it is easier while backporting.

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

@BridgeAR BridgeAR mentioned this pull request Apr 2, 2018
4 tasks
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 2, 2018
A error message should always be non-enumerable. This makes sure
that is true for dns errors as well. It also adds another check
in `common.expectsError` to make sure no other regressions are
introduced going forward.

Fixes nodejs#19716
@BridgeAR
Copy link
Member Author

BridgeAR commented Apr 4, 2018

Rebased due to conflicts. CI before landing: https://ci.nodejs.org/job/node-test-pull-request/14039/

BridgeAR added a commit to BridgeAR/node that referenced this pull request Apr 5, 2018
A error message should always be non-enumerable. This makes sure
that is true for dns errors as well. It also adds another check
in `common.expectsError` to make sure no other regressions are
introduced going forward.

Fixes nodejs#19716

PR-URL: nodejs#19719
Fixes: nodejs#19716
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR
Copy link
Member Author

BridgeAR commented Apr 5, 2018

Landed in 22da2f7

@BridgeAR BridgeAR closed this Apr 5, 2018
@alexjeffburke
Copy link
Contributor

Awesome that it landed! Thanks all.

@targos targos added backport-requested-v9.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 6, 2018
@targos
Copy link
Member

targos commented Apr 6, 2018

This needs a backport PR for v9.x-staging.

BridgeAR added a commit to BridgeAR/node that referenced this pull request May 1, 2018
A error message should always be non-enumerable. This makes sure
that is true for dns errors as well. It also adds another check
in `common.expectsError` to make sure no other regressions are
introduced going forward.

Fixes nodejs#19716

PR-URL: nodejs#19719
Fixes: nodejs#19716
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung pushed a commit to joyeecheung/node that referenced this pull request May 2, 2018
A error message should always be non-enumerable. This makes sure
that is true for dns errors as well. It also adds another check
in `common.expectsError` to make sure no other regressions are
introduced going forward.

Fixes nodejs#19716

PR-URL: nodejs#19719
Fixes: nodejs#19716
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 22, 2018
A error message should always be non-enumerable. This makes sure
that is true for dns errors as well. It also adds another check
in `common.expectsError` to make sure no other regressions are
introduced going forward.

Fixes #19716

Backport-PR-URL: #19191
PR-URL: #19719
Fixes: #19716
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
A error message should always be non-enumerable. This makes sure
that is true for dns errors as well. It also adds another check
in `common.expectsError` to make sure no other regressions are
introduced going forward.

Fixes #19716

Backport-PR-URL: #19191
PR-URL: #19719
Fixes: #19716
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 9, 2018
rvagg pushed a commit that referenced this pull request Aug 16, 2018
A error message should always be non-enumerable. This makes sure
that is true for dns errors as well. It also adds another check
in `common.expectsError` to make sure no other regressions are
introduced going forward.

Fixes #19716

Backport-PR-URL: #19191
PR-URL: #19719
Fixes: #19716
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR deleted the fix-error-enumerablility branch April 1, 2019 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

8 participants