-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
Fix error enumerablility #19719
Conversation
There was a problem hiding this 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
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? |
@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. |
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/ |
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. |
33c5f5e
to
e6af409
Compare
I updated the PR to only fix the bug and moved the second commit to another PR so it is easier while backporting. |
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
e6af409
to
3f1ff96
Compare
Rebased due to conflicts. CI before landing: https://ci.nodejs.org/job/node-test-pull-request/14039/ |
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>
Landed in 22da2f7 |
Awesome that it landed! Thanks all. |
This needs a backport PR for v9.x-staging. |
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>
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>
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>
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>
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>
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 areintroduced going forward.
Fixes: #19716
2. Commit:Right now the hostname could in some cases be missed, depending onthe 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), orvcbuild test
(Windows) passes