Skip to content

Commit

Permalink
Handle unhandledRejections, help with cache eacces
Browse files Browse the repository at this point in the history
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.

PR-URL: #227
Credit: @isaacs
Close: #227
Reviewed-by: @isaacs
  • Loading branch information
isaacs committed Aug 2, 2019
1 parent 99edd49 commit 5b38902
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 15 deletions.
1 change: 1 addition & 0 deletions bin/npm-cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@
log.info('using', 'node@%s', process.version)

process.on('uncaughtException', errorHandler)
process.on('unhandledRejection', errorHandler)

if (conf.usage && npm.command !== 'help') {
npm.argv.unshift(npm.command)
Expand Down
7 changes: 4 additions & 3 deletions lib/utils/error-handler.js
Original file line number Diff line number Diff line change
Expand Up @@ -187,11 +187,12 @@ function errorHandler (er) {
log.verbose('npm ', 'v' + npm.version)

;[
'code',
'syscall',
'file',
'path',
'code',
'errno',
'syscall'
'dest',
'errno'
].forEach(function (k) {
var v = er[k]
if (v) log.error(k, v)
Expand Down
49 changes: 37 additions & 12 deletions lib/utils/error-message.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
var npm = require('../npm.js')
var util = require('util')
var nameValidator = require('validate-npm-package-name')
var npmlog = require('npmlog')

module.exports = errorMessage

Expand Down Expand Up @@ -33,18 +34,42 @@ function errorMessage (er) {

case 'EACCES':
case 'EPERM':
short.push(['', er])
detail.push([
'',
[
'\nThe operation was rejected by your operating system.',
(process.platform === 'win32'
? 'It\'s possible that the file was already in use (by a text editor or antivirus),\nor that you lack permissions to access it.'
: 'It is likely you do not have the permissions to access this file as the current user'),
'\nIf you believe this might be a permissions issue, please double-check the',
'permissions of the file and its containing directories, or try running',
'the command again as root/Administrator (though this is not recommended).'
].join('\n')])
const isCachePath = typeof er.path === 'string' &&
er.path.startsWith(npm.config.get('cache'))
const isCacheDest = typeof er.dest === 'string' &&
er.dest.startsWith(npm.config.get('cache'))

const isWindows = process.platform === 'win32'

if (!isWindows && (isCachePath || isCacheDest)) {
// user probably doesn't need this, but still add it to the debug log
npmlog.verbose(er.stack)
short.push([
'',
[
'',
'Your cache folder contains root-owned files, due to a bug in',
'previous versions of npm which has since been addressed.',
'',
'To permanently fix this problem, please run:',
` sudo chown -R ${process.getuid()}:${process.getgid()} ${JSON.stringify(npm.config.get('cache'))}`
].join('\n')
])
} else {
short.push(['', er])
detail.push([
'',
[
'\nThe operation was rejected by your operating system.',
(process.platform === 'win32'
? 'It\'s possible that the file was already in use (by a text editor or antivirus),\n' +
'or that you lack permissions to access it.'
: 'It is likely you do not have the permissions to access this file as the current user'),
'\nIf you believe this might be a permissions issue, please double-check the',
'permissions of the file and its containing directories, or try running',
'the command again as root/Administrator.'
].join('\n')])
}
break

case 'ELIFECYCLE':
Expand Down
38 changes: 38 additions & 0 deletions test/tap/cache-eacces-error-message.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
const npm = require('../../lib/npm.js')
const t = require('tap')

if (process.platform === 'win32') {
t.plan(0, 'this is a unix-only thing')
process.exit(0)
}

const errorMessage = require('../../lib/utils/error-message.js')

const common = require('../common-tap.js')

t.plan(1)

npm.load({ cache: common.cache }, () => {
npm.config.set('cache', common.cache)
const er = new Error('access is e, i am afraid')
er.code = 'EACCES'
er.errno = -13
er.path = common.cache + '/src'
er.dest = common.cache + '/to'

t.match(errorMessage(er), {
summary: [
[
'',
new RegExp('\n' +
'Your cache folder contains root-owned files, due to a bug in\n' +
'previous versions of npm which has since been addressed.\n' +
'\n' +
'To permanently fix this problem, please run:\n' +
' sudo chown -R [0-9]+:[0-9]+ ".*npm_cache_cache-eacces-error-message"'
)
]
],
detail: []
}, 'get the helpful error message')
})

0 comments on commit 5b38902

Please # to comment.