Skip to content

Commit

Permalink
fix(npm-lifecycle): properly handle SIGINT from child process (#41)
Browse files Browse the repository at this point in the history
When a child process sends the INT signal, pnpm should not assume child failed to terminate

Closes pnpm/pnpm#7164

Co-authored-by: Yanick Rochon <yanick.rochon@dynamicly.com>
  • Loading branch information
yanickrochon and Yanick Rochon committed Oct 15, 2023
1 parent 99ac042 commit d495284
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 6 deletions.
4 changes: 2 additions & 2 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ function runCmd_ (cmd, pkg, env, wd, opts, stage, unsafe, uid, gid, cb_) {
proc.on('close', (code, signal) => {
opts.log.silly('lifecycle', logid(pkg, stage), 'Returned: code:', code, ' signal:', signal)
let err
if (signal) {
if (signal && signal !== "SIGINT") {
err = new PnpmError('CHILD_PROCESS_FAILED', `Command failed with signal "${signal}"`)
process.kill(process.pid, signal)
} else if (code) {
Expand All @@ -304,7 +304,7 @@ function runCmd_ (cmd, pkg, env, wd, opts, stage, unsafe, uid, gid, cb_) {
if (er.code !== 'EPERM') {
er.code = 'ELIFECYCLE'
}
fs.stat(opts.dir, (statError, d) => {
fs.stat(opts.dir, (statError) => {
if (statError && statError.code === 'ENOENT' && opts.dir.split(path.sep).slice(-1)[0] === 'node_modules') {
opts.log.warn('', 'Local package.json exists, but node_modules missing, did you mean to install?')
}
Expand Down
3 changes: 2 additions & 1 deletion test/fixtures/count-to-10/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"version": "1.0.0",
"scripts": {
"postinstall": "node postinstall",
"signal-exit": "echo 'signal-exit script' && kill -s ABRT $$"
"signal-abrt": "echo 'signal-exit script' && kill -s ABRT $$",
"signal-int": "echo 'signal-int script' && kill -s INT $$"
}
}
65 changes: 62 additions & 3 deletions test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -183,13 +183,72 @@ test('throw error signal kills child', async function (t) {
const dir = fixture
const pkg = require(path.join(fixture, 'package.json'))

t.rejects(async () => {
await lifecycle(pkg, 'signal-exit', fixture, {
await t.rejects(async () => {
await lifecycle(pkg, 'signal-abrt', fixture, {
stdio: 'pipe',
log,
dir,
config: {}
})
stubProcessExit.restore()
})

stubProcessExit.restore()
})

test('no error on INT signal from child', async function (t) {
const fixture = path.join(__dirname, 'fixtures', 'count-to-10')

const verbose = sinon.spy()
const silly = sinon.spy()
const info = sinon.spy()

const stubProcessExit = sinon.stub(process, 'kill').callsFake(noop)

const log = {
level: 'silent',
info,
warn: noop,
silly,
verbose,
pause: noop,
resume: noop,
clearProgress: noop,
showProgress: noop
}

const dir = fixture
const pkg = require(path.join(fixture, 'package.json'))

await t.resolves(async () => {
await lifecycle(pkg, 'signal-int', fixture, {
stdio: 'pipe',
log,
dir,
config: {}
})
})

stubProcessExit.restore()

t.ok(
!info.calledWithMatch(
'lifecycle',
'undefined~signal-int:',
'Failed to exec signal-int script'
),
'INT signal intercepted incorrectly'
)

t.ok(
silly.calledWithMatch(
'lifecycle',
'undefined~signal-int:',
'Returned: code:',
null,
' signal:',
'SIGINT'
),
'INT signal reported'
)

})

0 comments on commit d495284

Please sign in to comment.