Skip to content

fix: reject with error from parent context on close#102

Merged
lukekarrys merged 3 commits intomainfrom
lk/close-stack-trace
Apr 15, 2024
Merged

fix: reject with error from parent context on close#102
lukekarrys merged 3 commits intomainfrom
lk/close-stack-trace

Conversation

@lukekarrys
Copy link
Contributor

@lukekarrys lukekarrys commented Apr 14, 2024 •
edited
Loading

This PR does two things:

  • When the spawned process is closed with a code or signal, it rejects with an error from the parent context to preserve that more useful stack trace
  • Changes the error message from command failed to @npmcli/promise-spawn command failed. I feel like command failed looks too generic when thrown in the context of the npm CLI and this makes it clear that the error came from spawning something. I removed this from the PR since it could be a breaking change since npm looks for this error message. It would be better to give this a better message further up the stack.

It also refactors a bit to remove one level of nesting. It uses the Promise.withResolvers() pattern to hoist the resolve and reject functions since none of the work actually needs to be done within the Promise executor.

Example

Here's a before and after of npm rebuild --loglevel verbose of a failing preinstall script. Some output is removed for clarity/brevity.

before

npm verb title npm rebuild
npm verb argv "rebuild" "--loglevel" "verbose"
npm info run debug-init-cancel@1.0.0 preinstall  bash ./my-preinstall-script.sh
npm info run debug-init-cancel@1.0.0 preinstall { code: 1, signal: null }
npm verb stack Error: command failed
npm verb stack     at ChildProcess.<anonymous> (/Users/lukekarrys/Desktop/npm-debug/.versions/npm-10.5.2/node_modules/@npmcli/promise-spawn/lib/index.js:53:27)
npm verb stack     at ChildProcess.emit (node:events:518:28)
npm verb stack     at maybeClose (node:internal/child_process:1105:16)
npm verb stack     at Socket.<anonymous> (node:internal/child_process:457:11)
npm verb stack     at Socket.emit (node:events:518:28)
npm verb stack     at Pipe.<anonymous> (node:net:337:12)
npm verb pkgid debug-init-cancel@1.0.0
npm ERR! code 1
npm ERR! path /Users/lukekarrys/Desktop/npm-debug/debug-init-cancel
npm ERR! command failed
npm ERR! command sh -c bash ./my-preinstall-script.sh
npm ERR! Somes script output
npm ERR! Oh no, this will error!
npm verb exit 1
npm verb code 1

after

npm verb title npm rebuild
npm verb argv "rebuild" "--loglevel" "verbose"
npm info run debug-init-cancel@1.0.0 preinstall  bash ./my-preinstall-script.sh
npm info run debug-init-cancel@1.0.0 preinstall { code: 1, signal: null }
npm verb stack Error: command failed
npm verb stack     at promiseSpawn (/Users/lukekarrys/projects/npm/cli/node_modules/@npmcli/promise-spawn/lib/index.js:22:22)
npm verb stack     at spawnWithShell (/Users/lukekarrys/projects/npm/cli/node_modules/@npmcli/promise-spawn/lib/index.js:124:10)
npm verb stack     at promiseSpawn (/Users/lukekarrys/projects/npm/cli/node_modules/@npmcli/promise-spawn/lib/index.js:12:12)
npm verb stack     at runScriptPkg (/Users/lukekarrys/projects/npm/cli/node_modules/@npmcli/run-script/lib/run-script-pkg.js:75:13)
npm verb stack     at runScript (/Users/lukekarrys/projects/npm/cli/node_modules/@npmcli/run-script/lib/run-script.js:9:12)
npm verb stack     at /Users/lukekarrys/projects/npm/cli/workspaces/arborist/lib/arborist/rebuild.js:333:17
npm verb stack     at run (/Users/lukekarrys/projects/npm/cli/node_modules/promise-call-limit/dist/commonjs/index.js:67:22)
npm verb stack     at /Users/lukekarrys/projects/npm/cli/node_modules/promise-call-limit/dist/commonjs/index.js:84:9
npm verb stack     at new Promise (<anonymous>)
npm verb stack     at callLimit (/Users/lukekarrys/projects/npm/cli/node_modules/promise-call-limit/dist/commonjs/index.js:35:69)
npm verb pkgid debug-init-cancel@1.0.0
npm ERR! code 1
npm ERR! path /Users/lukekarrys/Desktop/npm-debug/debug-init-cancel
npm ERR! command failed
npm ERR! command sh -c bash ./my-preinstall-script.sh
npm ERR! Some script output
npm ERR! Oh no, this will error!
npm verb exit 1
npm verb code 1

@lukekarrys lukekarrys requested a review from a team as a code owner April 14, 2024 21:15
github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 14, 2024
View reviewed changes
lib/index.js
}

proc.on('error', reject)
const proc = spawn(cmd, args, opts)

Check warning

Code scanning / CodeQL

Unsafe shell command constructed from library input

This shell argument which depends on [library input](1) is later used in a [shell command](2). This shell argument which depends on [library input](3) is later used in a [shell command](2). This shell argument which depends on [library input](4) is later used in a [shell command](2). This shell argument which depends on [library input](5) is later used in a [shell command](2).
@lukekarrys lukekarrys changed the title fix: create close error in root function for better stack trace fix: reject with error from parent context on close Apr 14, 2024
wraithgar
wraithgar reviewed Apr 15, 2024
View reviewed changes
@wraithgar
Copy link
Member

wraithgar commented Apr 15, 2024

Everything looks good here, I get why we're doing it, but it still makes me nervous in light of things like npm/run-script#188. Let's be extra wary of new issues we didn't expect.

@lukekarrys lukekarrys merged commit 4912015 into main Apr 15, 2024
@lukekarrys lukekarrys deleted the lk/close-stack-trace branch April 15, 2024 02:42
@github-actions github-actions bot mentioned this pull request Apr 15, 2024
Closed
@github-actions github-actions bot mentioned this pull request May 4, 2024
Merged
lukekarrys pushed a commit that referenced this pull request May 4, 2024
@github-actions
chore: release 7.0.2 (#107)
7928731 
🤖 I have created a release *beep* *boop*
---


## [7.0.2](v7.0.1...v7.0.2)
(2024-05-04)

### Bug Fixes

*
[`4912015`](4912015)
[#102](#102) reject with error
from parent context on close (#102) (@lukekarrys)

### Chores

*
[`09872d7`](09872d7)
[#105](#105) linting:
no-unused-vars (@lukekarrys)
*
[`70f0eb7`](70f0eb7)
[#105](#105) bump
@npmcli/template-oss to 4.22.0 (@lukekarrys)
*
[`82ae2a7`](82ae2a7)
[#105](#105) postinstall for
dependabot template-oss PR (@lukekarrys)
*
[`2855879`](2855879)
[#104](#104) bump
@npmcli/template-oss from 4.21.3 to 4.21.4 (@dependabot[bot])

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wraithgar wraithgar wraithgar approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@lukekarrys @wraithgar