Skip to content

Commit 1e375c1

Browse files
lukekarryslukekarrys
authored
feat: create exit handler class (#7442)
This PR refactors `exit-handler.js` to be a class so that it can more easily track its internal state. It uses this state to now fully distinguish between 3 states: npm never being set, npm not loaded, and exit handler never called. There are some new error messages shown via console.error if we know we are in an unexpected state. This also continues the refactoring started in #7415 to separate concerns between `npm` and `CLI`. Identifying the error message and logging it have been move to `npm` but catching that error and setting the `process.exitCode` are still handled in `exit-handler.js` and `cli/entry.js`. It also moves `command.cmdExec` to `npm` since it never called from within any `command` instance. This lets `npm` only ever call `command.exec` or `command.workspaceExec` now.
1 parent ca1a68d commit 1e375c1

File tree

14 files changed

+655
-451
lines changed

14 files changed

+655
-451
lines changed

‎lib/base-cmd.js

+1-28
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
const { log, output } = require('proc-log')
1+
const { log } = require('proc-log')
22

33
class BaseCommand {
44
static workspaces = false
@@ -111,33 +111,6 @@ class BaseCommand {
111111
})
112112
}
113113

114-
async cmdExec (args) {
115-
const { config } = this.npm
116-
117-
if (config.get('usage')) {
118-
return output.standard(this.usage)
119-
}
120-
121-
const hasWsConfig = config.get('workspaces') || config.get('workspace').length
122-
// if cwd is a workspace, the default is set to [that workspace]
123-
const implicitWs = config.get('workspace', 'default').length
124-
125-
// (-ws || -w foo) && (cwd is not a workspace || command is not ignoring implicit workspaces)
126-
if (hasWsConfig && (!implicitWs || !this.constructor.ignoreImplicitWorkspace)) {
127-
if (this.npm.global) {
128-
throw new Error('Workspaces not supported for global packages')
129-
}
130-
if (!this.constructor.workspaces) {
131-
throw Object.assign(new Error('This command does not support workspaces.'), {
132-
code: 'ENOWORKSPACES',
133-
})
134-
}
135-
return this.execWorkspaces(args)
136-
}
137-
138-
return this.exec(args)
139-
}
140-
141114
// Compare the number of entries with what was expected
142115
checkExpected (entries) {
143116
if (!this.npm.config.isDefault('expect-results')) {

‎lib/cli/entry.js

+7-15
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,8 @@ module.exports = async (process, validateEngines) => {
1515
require('graceful-fs').gracefulify(require('fs'))
1616

1717
const satisfies = require('semver/functions/satisfies')
18-
const exitHandler = require('./exit-handler.js')
18+
const ExitHandler = require('./exit-handler.js')
19+
const exitHandler = new ExitHandler({ process })
1920
const Npm = require('../npm.js')
2021
const npm = new Npm()
2122
exitHandler.setNpm(npm)
@@ -28,8 +29,7 @@ module.exports = async (process, validateEngines) => {
2829

2930
// At this point we've required a few files and can be pretty sure we dont contain invalid syntax for this version of node. It's possible a lazy require would, but that's unlikely enough that it's not worth catching anymore and we attach the more important exit handlers.
3031
validateEngines.off()
31-
process.on('uncaughtException', exitHandler)
32-
process.on('unhandledRejection', exitHandler)
32+
exitHandler.registerUncaughtHandlers()
3333

3434
// It is now safe to log a warning if they are using a version of node that is not going to fail on syntax errors but is still unsupported and untested and might not work reliably. This is safe to use the logger now which we want since this will show up in the error log too.
3535
if (!satisfies(validateEngines.node, validateEngines.engines)) {
@@ -42,13 +42,13 @@ module.exports = async (process, validateEngines) => {
4242
const { exec, command, args } = await npm.load()
4343

4444
if (!exec) {
45-
return exitHandler()
45+
return exitHandler.exit()
4646
}
4747

4848
if (!command) {
4949
output.standard(npm.usage)
5050
process.exitCode = 1
51-
return exitHandler()
51+
return exitHandler.exit()
5252
}
5353

5454
// Options are prefixed by a hyphen-minus (-, \u2d).
@@ -72,16 +72,8 @@ module.exports = async (process, validateEngines) => {
7272
updateNotifier(npm).then((msg) => (npm.updateNotification = msg))
7373

7474
await execPromise
75-
return exitHandler()
75+
return exitHandler.exit()
7676
} catch (err) {
77-
if (err.code === 'EUNKNOWNCOMMAND') {
78-
const didYouMean = require('../utils/did-you-mean.js')
79-
const suggestions = await didYouMean(npm.localPrefix, err.command)
80-
output.standard(`Unknown command: "${err.command}"${suggestions}\n`)
81-
output.standard('To see a list of supported npm commands, run:\n npm help')
82-
process.exitCode = 1
83-
return exitHandler()
84-
}
85-
return exitHandler(err)
77+
return exitHandler.exit(err)
8678
}
8779
}

‎lib/cli/exit-handler.js

+138-125
Original file line numberDiff line numberDiff line change
@@ -1,160 +1,173 @@
11
const { log, output, META } = require('proc-log')
2-
const errorMessage = require('../utils/error-message.js')
3-
const { redactLog: replaceInfo } = require('@npmcli/redact')
2+
const { errorMessage, getExitCodeFromError } = require('../utils/error-message.js')
43

5-
let npm = null // set by the cli
6-
let exitHandlerCalled = false
7-
let showLogFileError = false
4+
class ExitHandler {
5+
#npm = null
6+
#process = null
7+
#exited = false
8+
#exitErrorMessage = false
89

9-
process.on('exit', code => {
10-
const hasLoadedNpm = npm?.config.loaded
10+
#noNpmError = false
1111

12-
if (!code) {
13-
log.info('ok')
14-
} else {
15-
log.verbose('code', code)
12+
get #hasNpm () {
13+
return !!this.#npm
1614
}
1715

18-
if (!exitHandlerCalled) {
19-
process.exitCode = code || 1
20-
log.error('', 'Exit handler never called!')
21-
log.error('', 'This is an error with npm itself. Please report this error at:')
22-
log.error('', ' <https://github.com/npm/cli/issues>')
23-
// eslint-disable-next-line no-console
24-
console.error('')
25-
showLogFileError = true
16+
get #loaded () {
17+
return !!this.#npm?.loaded
2618
}
2719

28-
// npm must be loaded to know where the log file was written
29-
if (hasLoadedNpm) {
30-
npm.finish({ showLogFileError })
31-
// This removes any listeners npm setup, mostly for tests to avoid max listener warnings
32-
npm.unload()
20+
get #showExitErrorMessage () {
21+
if (!this.#loaded) {
22+
return false
23+
}
24+
if (!this.#exited) {
25+
return true
26+
}
27+
return this.#exitErrorMessage
3328
}
3429

35-
// these are needed for the tests to have a clean slate in each test case
36-
exitHandlerCalled = false
37-
showLogFileError = false
38-
})
30+
get #notLoadedOrExited () {
31+
return !this.#loaded && !this.#exited
32+
}
3933

40-
const exitHandler = err => {
41-
exitHandlerCalled = true
34+
setNpm (npm) {
35+
this.#npm = npm
36+
}
4237

43-
const hasLoadedNpm = npm?.config.loaded
38+
constructor ({ process }) {
39+
this.#process = process
40+
this.#process.on('exit', this.#handleProcesExitAndReset)
41+
}
4442

45-
if (!npm) {
46-
err = err || new Error('Exit prior to setting npm in exit handler')
47-
// Don't use proc-log here since npm was never set
48-
// eslint-disable-next-line no-console
49-
console.error(err.stack || err.message)
50-
return process.exit(1)
43+
registerUncaughtHandlers () {
44+
this.#process.on('uncaughtException', this.#handleExit)
45+
this.#process.on('unhandledRejection', this.#handleExit)
5146
}
5247

53-
if (!hasLoadedNpm) {
54-
err = err || new Error('Exit prior to config file resolving.')
55-
// Don't use proc-log here since npm was never loaded
56-
// eslint-disable-next-line no-console
57-
console.error(err.stack || err.message)
48+
exit (err) {
49+
this.#handleExit(err)
5850
}
5951

60-
// only show the notification if it finished.
61-
if (typeof npm.updateNotification === 'string') {
62-
log.notice('', npm.updateNotification, { [META]: true, force: true })
52+
#handleProcesExitAndReset = (code) => {
53+
this.#handleProcessExit(code)
54+
55+
// Reset all the state. This is only relevant for tests since
56+
// in reality the process fully exits here.
57+
this.#process.off('exit', this.#handleProcesExitAndReset)
58+
this.#process.off('uncaughtException', this.#handleExit)
59+
this.#process.off('unhandledRejection', this.#handleExit)
60+
if (this.#loaded) {
61+
this.#npm.unload()
62+
}
63+
this.#npm = null
64+
this.#exited = false
65+
this.#exitErrorMessage = false
6366
}
6467

65-
let exitCode = process.exitCode || 0
66-
let noLogMessage = exitCode !== 0
67-
let jsonError
68-
69-
if (err) {
70-
exitCode = 1
71-
// if we got a command that just shells out to something else, then it
72-
// will presumably print its own errors and exit with a proper status
73-
// code if there's a problem. If we got an error with a code=0, then...
74-
// something else went wrong along the way, so maybe an npm problem?
75-
const isShellout = npm.isShellout
76-
const quietShellout = isShellout && typeof err.code === 'number' && err.code
77-
if (quietShellout) {
78-
exitCode = err.code
79-
noLogMessage = true
80-
} else if (typeof err === 'string') {
81-
// XXX: we should stop throwing strings
82-
log.error('', err)
83-
noLogMessage = true
84-
} else if (!(err instanceof Error)) {
85-
log.error('weird error', err)
86-
noLogMessage = true
87-
} else {
88-
const os = require('node:os')
89-
const fs = require('node:fs')
90-
if (!err.code) {
91-
const matchErrorCode = err.message.match(/^(?:Error: )?(E[A-Z]+)/)
92-
err.code = matchErrorCode && matchErrorCode[1]
93-
}
68+
#handleProcessExit (code) {
69+
// Force exit code to a number if it has not been set
70+
const exitCode = typeof code === 'number' ? code : (this.#exited ? 0 : 1)
71+
this.#process.exitCode = exitCode
9472

95-
for (const k of ['type', 'stack', 'statusCode', 'pkgid']) {
96-
const v = err[k]
97-
if (v) {
98-
log.verbose(k, replaceInfo(v))
99-
}
100-
}
73+
if (this.#notLoadedOrExited) {
74+
// Exit handler was not called and npm was not loaded so we have to log something
75+
this.#logConsoleError(new Error(`Process exited unexpectedly with code: ${exitCode}`))
76+
return
77+
}
10178

102-
log.verbose('cwd', process.cwd())
103-
log.verbose('', os.type() + ' ' + os.release())
104-
log.verbose('node', process.version)
105-
log.verbose('npm ', 'v' + npm.version)
79+
if (this.#logNoNpmError()) {
80+
return
81+
}
10682

107-
for (const k of ['code', 'syscall', 'file', 'path', 'dest', 'errno']) {
108-
const v = err[k]
109-
if (v) {
110-
log.error(k, v)
111-
}
112-
}
83+
const os = require('node:os')
84+
log.verbose('cwd', this.#process.cwd())
85+
log.verbose('os', `${os.type()} ${os.release()}`)
86+
log.verbose('node', this.#process.version)
87+
log.verbose('npm ', `v${this.#npm.version}`)
11388

114-
const { summary, detail, json, files = [] } = errorMessage(err, npm)
115-
jsonError = json
116-
117-
for (let [file, content] of files) {
118-
file = `${npm.logPath}${file}`
119-
content = `'Log files:\n${npm.logFiles.join('\n')}\n\n${content.trim()}\n`
120-
try {
121-
fs.writeFileSync(file, content)
122-
detail.push(['', `\n\nFor a full report see:\n${file}`])
123-
} catch (logFileErr) {
124-
log.warn('', `Could not write error message to ${file} due to ${logFileErr}`)
125-
}
126-
}
89+
// only show the notification if it finished
90+
if (typeof this.#npm.updateNotification === 'string') {
91+
log.notice('', this.#npm.updateNotification, { [META]: true, force: true })
92+
}
12793

128-
for (const errline of [...summary, ...detail]) {
129-
log.error(...errline)
94+
if (!this.#exited) {
95+
log.error('', 'Exit handler never called!')
96+
log.error('', 'This is an error with npm itself. Please report this error at:')
97+
log.error('', ' <https://github.com/npm/cli/issues>')
98+
if (this.#npm.silent) {
99+
output.error('')
130100
}
101+
}
131102

132-
if (typeof err.errno === 'number') {
133-
exitCode = err.errno
134-
} else if (typeof err.code === 'number') {
135-
exitCode = err.code
136-
}
103+
log.verbose('exit', exitCode)
104+
105+
if (exitCode) {
106+
log.verbose('code', exitCode)
107+
} else {
108+
log.info('ok')
109+
}
110+
111+
if (this.#showExitErrorMessage) {
112+
log.error('', this.#npm.exitErrorMessage())
137113
}
138114
}
139115

140-
if (hasLoadedNpm) {
141-
output.flush({ [META]: true, jsonError })
116+
#logConsoleError (err) {
117+
// Run our error message formatters on all errors even if we
118+
// have no npm or an unloaded npm. This will clean the error
119+
// and possible return a formatted message about EACCESS or something.
120+
const { summary, detail } = errorMessage(err, this.#npm)
121+
const formatted = [...new Set([...summary, ...detail].flat().filter(Boolean))].join('\n')
122+
// If we didn't get anything from the formatted message then just display the full stack
123+
// eslint-disable-next-line no-console
124+
console.error(formatted === err.message ? err.stack : formatted)
142125
}
143126

144-
log.verbose('exit', exitCode || 0)
127+
#logNoNpmError (err) {
128+
if (this.#hasNpm) {
129+
return false
130+
}
131+
// Make sure we only log this error once
132+
if (!this.#noNpmError) {
133+
this.#noNpmError = true
134+
this.#logConsoleError(
135+
new Error(`Exit prior to setting npm in exit handler`, err ? { cause: err } : {})
136+
)
137+
}
138+
return true
139+
}
140+
141+
#handleExit = (err) => {
142+
this.#exited = true
143+
144+
// No npm at all
145+
if (this.#logNoNpmError(err)) {
146+
return this.#process.exit(this.#process.exitCode || getExitCodeFromError(err) || 1)
147+
}
145148

146-
showLogFileError = (hasLoadedNpm && npm.silent) || noLogMessage
147-
? false
148-
: !!exitCode
149+
// npm was never loaded but we still might have a config loading error or
150+
// something similar that we can run through the error message formatter
151+
// to give the user a clue as to what happened.s
152+
if (!this.#loaded) {
153+
this.#logConsoleError(new Error('Exit prior to config file resolving', { cause: err }))
154+
return this.#process.exit(this.#process.exitCode || getExitCodeFromError(err) || 1)
155+
}
156+
157+
this.#exitErrorMessage = err?.suppressError === true ? false : !!err
149158

150-
// explicitly call process.exit now so we don't hang on things like the
151-
// update notifier, also flush stdout/err beforehand because process.exit doesn't
152-
// wait for that to happen.
153-
let flushed = 0
154-
const flush = [process.stderr, process.stdout]
155-
const exit = () => ++flushed === flush.length && process.exit(exitCode)
156-
flush.forEach((f) => f.write('', exit))
159+
// Prefer the exit code of the error, then the current process exit code,
160+
// then set it to 1 if we still have an error. Otherwise we call process.exit
161+
// with undefined so that it can determine the final exit code
162+
const exitCode = err?.exitCode ?? this.#process.exitCode ?? (err ? 1 : undefined)
163+
164+
// explicitly call process.exit now so we don't hang on things like the
165+
// update notifier, also flush stdout/err beforehand because process.exit doesn't
166+
// wait for that to happen.
167+
this.#process.stderr.write('', () => this.#process.stdout.write('', () => {
168+
this.#process.exit(exitCode)
169+
}))
170+
}
157171
}
158172

159-
module.exports = exitHandler
160-
module.exports.setNpm = n => (npm = n)
173+
module.exports = ExitHandler

0 commit comments

Comments
 (0)