-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Handle unhandledRejections, help with cache eacces #227
Handle unhandledRejections, help with cache eacces #227
Conversation
Suggested by @godmar in https://npm.community/t/npm-err-cb-never-called-permission-denied/9167/5 Incidentally, this turned up that we're catching uncaughtExceptions in the main npm functions, but not unhandledRejections! Tracing this through, it seems like node-fetch-npm's use of cacache is particularly brittle. Any throw that comes from cacache is not caught properly, since node-fetch-npm is all streams and callbacks. The naive approach (just adding a catch and failing the callback) doesn't work, because then make-fetch-happen and npm-registry-fetch interpret the failure as an invalid response, when actually it was a local cache error. So, a bit more love and polish is definitely still needed in the guts of npm's fetching and caching code paths. In the meantime, though, handling any unhandledRejection at the top level prevents at least the worst and most useless type of error message.
This doesn't make a difference. The default handler for unhandledRejection throws and ends up in |
It does make a difference. The default handler for The difference in practice:
Without the explicit |
I had noticed the lack of unhandledRejection when debugging this myself. You're right that in order to execute your errorHandler, you have to set it. It hadn't made a difference for me with the old error handler - in both case, you got a stack trace. |
Right, the other bit of the fix is in |
649e3c8
to
897537a
Compare
BUGFIXES * [`27cccfbda`](npm/cli@27cccfb) [nodejs#223](npm/cli#223) vulns → vulnerabilities in npm audit output ([@sapegin](https://github.com/sapegin)) * [`d5e865eb7`](npm/cli@d5e865e) [nodejs#222](npm/cli#222) [nodejs#226](npm/cli#226) install, doctor: don't crash if registry unset ([@dmitrydvorkin](https://github.com/dmitrydvorkin), [@isaacs](https://github.com/isaacs)) * [`5b3890226`](npm/cli@5b38902) [nodejs#227](npm/cli#227) [npm.community#9167](https://npm.community/t/npm-err-cb-never-called-permission-denied/9167/5) Handle unhandledRejections, tell user what to do when encountering an `EACCES` error in the cache. ([@isaacs](https://github.com/isaacs)) DEPENDENCIES * [`77516df6e`](npm/cli@77516df) `licensee@7.0.3` ([@isaacs](https://github.com/isaacs)) * [`ceb993590`](npm/cli@ceb9935) `query-string@6.8.2` ([@isaacs](https://github.com/isaacs)) * [`4050b9189`](npm/cli@4050b91) `hosted-git-info@2.8.2` * [nodejs#46](npm/hosted-git-info#46) [nodejs#43](npm/hosted-git-info#43) [nodejs#47](npm/hosted-git-info#47) [nodejs#44](npm/hosted-git-info#44) Add support for GitLab subgroups ([@mterrel](https://github.com/mterrel), [@isaacs](https://github.com/isaacs), [@ybiquitous](https://github.com/ybiquitous)) * [`3b1d629`](npm/hosted-git-info@3b1d629) [nodejs#48](npm/hosted-git-info#48) fix http protocol using sshurl by default ([@fengmk2](https://github.com/fengmk2)) * [`5d4a8d7`](npm/hosted-git-info@5d4a8d7) ignore noCommittish on tarball url generation ([@isaacs](https://github.com/isaacs)) * [`1692435`](npm/hosted-git-info@1692435) use gist tarball url that works for anonymous gists ([@isaacs](https://github.com/isaacs)) * [`d5cf830`](npm/hosted-git-info@d5cf830) Do not allow invalid gist urls ([@isaacs](https://github.com/isaacs)) * [`e518222`](npm/hosted-git-info@e518222) Use LRU cache to prevent unbounded memory consumption ([@iarna](https://github.com/iarna))
BUGFIXES * [`27cccfbda`](npm/cli@27cccfb) [#223](npm/cli#223) vulns → vulnerabilities in npm audit output ([@sapegin](https://github.com/sapegin)) * [`d5e865eb7`](npm/cli@d5e865e) [#222](npm/cli#222) [#226](npm/cli#226) install, doctor: don't crash if registry unset ([@dmitrydvorkin](https://github.com/dmitrydvorkin), [@isaacs](https://github.com/isaacs)) * [`5b3890226`](npm/cli@5b38902) [#227](npm/cli#227) [npm.community#9167](https://npm.community/t/npm-err-cb-never-called-permission-denied/9167/5) Handle unhandledRejections, tell user what to do when encountering an `EACCES` error in the cache. ([@isaacs](https://github.com/isaacs)) DEPENDENCIES * [`77516df6e`](npm/cli@77516df) `licensee@7.0.3` ([@isaacs](https://github.com/isaacs)) * [`ceb993590`](npm/cli@ceb9935) `query-string@6.8.2` ([@isaacs](https://github.com/isaacs)) * [`4050b9189`](npm/cli@4050b91) `hosted-git-info@2.8.2` * [#46](npm/hosted-git-info#46) [#43](npm/hosted-git-info#43) [#47](npm/hosted-git-info#47) [#44](npm/hosted-git-info#44) Add support for GitLab subgroups ([@mterrel](https://github.com/mterrel), [@isaacs](https://github.com/isaacs), [@ybiquitous](https://github.com/ybiquitous)) * [`3b1d629`](npm/hosted-git-info@3b1d629) [#48](npm/hosted-git-info#48) fix http protocol using sshurl by default ([@fengmk2](https://github.com/fengmk2)) * [`5d4a8d7`](npm/hosted-git-info@5d4a8d7) ignore noCommittish on tarball url generation ([@isaacs](https://github.com/isaacs)) * [`1692435`](npm/hosted-git-info@1692435) use gist tarball url that works for anonymous gists ([@isaacs](https://github.com/isaacs)) * [`d5cf830`](npm/hosted-git-info@d5cf830) Do not allow invalid gist urls ([@isaacs](https://github.com/isaacs)) * [`e518222`](npm/hosted-git-info@e518222) Use LRU cache to prevent unbounded memory consumption ([@iarna](https://github.com/iarna)) PR-URL: #29023 Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
BUGFIXES * [`27cccfbda`](npm/cli@27cccfb) [#223](npm/cli#223) vulns → vulnerabilities in npm audit output ([@sapegin](https://github.com/sapegin)) * [`d5e865eb7`](npm/cli@d5e865e) [#222](npm/cli#222) [#226](npm/cli#226) install, doctor: don't crash if registry unset ([@dmitrydvorkin](https://github.com/dmitrydvorkin), [@isaacs](https://github.com/isaacs)) * [`5b3890226`](npm/cli@5b38902) [#227](npm/cli#227) [npm.community#9167](https://npm.community/t/npm-err-cb-never-called-permission-denied/9167/5) Handle unhandledRejections, tell user what to do when encountering an `EACCES` error in the cache. ([@isaacs](https://github.com/isaacs)) DEPENDENCIES * [`77516df6e`](npm/cli@77516df) `licensee@7.0.3` ([@isaacs](https://github.com/isaacs)) * [`ceb993590`](npm/cli@ceb9935) `query-string@6.8.2` ([@isaacs](https://github.com/isaacs)) * [`4050b9189`](npm/cli@4050b91) `hosted-git-info@2.8.2` * [#46](npm/hosted-git-info#46) [#43](npm/hosted-git-info#43) [#47](npm/hosted-git-info#47) [#44](npm/hosted-git-info#44) Add support for GitLab subgroups ([@mterrel](https://github.com/mterrel), [@isaacs](https://github.com/isaacs), [@ybiquitous](https://github.com/ybiquitous)) * [`3b1d629`](npm/hosted-git-info@3b1d629) [#48](npm/hosted-git-info#48) fix http protocol using sshurl by default ([@fengmk2](https://github.com/fengmk2)) * [`5d4a8d7`](npm/hosted-git-info@5d4a8d7) ignore noCommittish on tarball url generation ([@isaacs](https://github.com/isaacs)) * [`1692435`](npm/hosted-git-info@1692435) use gist tarball url that works for anonymous gists ([@isaacs](https://github.com/isaacs)) * [`d5cf830`](npm/hosted-git-info@d5cf830) Do not allow invalid gist urls ([@isaacs](https://github.com/isaacs)) * [`e518222`](npm/hosted-git-info@e518222) Use LRU cache to prevent unbounded memory consumption ([@iarna](https://github.com/iarna)) PR-URL: #29023 Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
BUGFIXES * [`27cccfbda`](npm/cli@27cccfb) [#223](npm/cli#223) vulns → vulnerabilities in npm audit output ([@sapegin](https://github.com/sapegin)) * [`d5e865eb7`](npm/cli@d5e865e) [#222](npm/cli#222) [#226](npm/cli#226) install, doctor: don't crash if registry unset ([@dmitrydvorkin](https://github.com/dmitrydvorkin), [@isaacs](https://github.com/isaacs)) * [`5b3890226`](npm/cli@5b38902) [#227](npm/cli#227) [npm.community#9167](https://npm.community/t/npm-err-cb-never-called-permission-denied/9167/5) Handle unhandledRejections, tell user what to do when encountering an `EACCES` error in the cache. ([@isaacs](https://github.com/isaacs)) DEPENDENCIES * [`77516df6e`](npm/cli@77516df) `licensee@7.0.3` ([@isaacs](https://github.com/isaacs)) * [`ceb993590`](npm/cli@ceb9935) `query-string@6.8.2` ([@isaacs](https://github.com/isaacs)) * [`4050b9189`](npm/cli@4050b91) `hosted-git-info@2.8.2` * [#46](npm/hosted-git-info#46) [#43](npm/hosted-git-info#43) [#47](npm/hosted-git-info#47) [#44](npm/hosted-git-info#44) Add support for GitLab subgroups ([@mterrel](https://github.com/mterrel), [@isaacs](https://github.com/isaacs), [@ybiquitous](https://github.com/ybiquitous)) * [`3b1d629`](npm/hosted-git-info@3b1d629) [#48](npm/hosted-git-info#48) fix http protocol using sshurl by default ([@fengmk2](https://github.com/fengmk2)) * [`5d4a8d7`](npm/hosted-git-info@5d4a8d7) ignore noCommittish on tarball url generation ([@isaacs](https://github.com/isaacs)) * [`1692435`](npm/hosted-git-info@1692435) use gist tarball url that works for anonymous gists ([@isaacs](https://github.com/isaacs)) * [`d5cf830`](npm/hosted-git-info@d5cf830) Do not allow invalid gist urls ([@isaacs](https://github.com/isaacs)) * [`e518222`](npm/hosted-git-info@e518222) Use LRU cache to prevent unbounded memory consumption ([@iarna](https://github.com/iarna)) PR-URL: #29023 Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Suggested by @godmar in
https://npm.community/t/npm-err-cb-never-called-permission-denied/9167/5
Incidentally, this turned up that we're catching uncaughtExceptions in
the main npm functions, but not unhandledRejections!
Tracing this through, it seems like node-fetch-npm's use of cacache is
particularly brittle. Any throw that comes from cacache is not caught
properly, since node-fetch-npm is all streams and callbacks. The naive
approach (just adding a catch and failing the callback) doesn't work,
because then make-fetch-happen and npm-registry-fetch interpret the
failure as an invalid response, when actually it was a local cache
error.
So, a bit more love and polish is definitely still needed in the
guts of npm's fetching and caching code paths. In the meantime, though,
handling any unhandledRejection at the top level prevents at least the
worst and most useless type of error message.