From 31d5a34de039a7a72d6c54ce7a8950edc1cd81b2 Mon Sep 17 00:00:00 2001 From: stelcheck Date: Thu, 7 Sep 2017 17:03:36 +0900 Subject: [PATCH 1/3] forward signals correctly signals are passed to the spawned child process; this means that the main process should not react to them, and instead wait for the spawned process to exit as a result of receiving the forwarded signal. --- src/bin.ts | 44 +++++++++++++++++++++++++++++++++++--------- src/index.spec.ts | 39 ++++++++++++++++++++++++++++++++++++++- tests/signals.ts | 12 ++++++++++++ 3 files changed, 85 insertions(+), 10 deletions(-) create mode 100644 tests/signals.ts diff --git a/src/bin.ts b/src/bin.ts index 8b8e76401..a3b864845 100644 --- a/src/bin.ts +++ b/src/bin.ts @@ -53,16 +53,42 @@ v8flags(function (err, v8flags) { const proc = spawn( process.execPath, nodeArgs.concat(join(__dirname, '_bin.js'), scriptArgs), - { stdio: 'inherit' } + { + // We need to run in detached mode so to avoid + // automatic propagation of signals to the child process. + // This is necessary because by default, keyboard interrupts + // are propagated to the process tree, but `kill` is not. + // + // See: https://nodejs.org/api/child_process.html#child_process_options_detached + detached: true, + + // Pipe all input and output to this process + stdio: 'inherit' + } ) - proc.on('exit', function (code: number, signal: string) { - process.on('exit', function () { - if (signal) { - process.kill(process.pid, signal) - } else { - process.exit(code) - } - }) + // Ignore signals, and instead forward them to the + // child process + const forward = (signal: NodeJS.Signals) => process.on(signal, () => proc.kill(signal)) + + // Interrupt (CTRL-C) + forward('SIGINT') + + // Termination (`kill` default signal) + forward('SIGTERM') + + // Terminal size change must be forwarded to the subprocess + forward('SIGWINCH') + + // On exit, exit this process with the same exit code + proc.on('close', (code: number, signal: string) => { + if (signal) { + process.kill(process.pid, signal) + } else if (code) { + process.exit(code) + } }) + + // If this process is exited, kill the child first + process.on('exit', (_code: number) => proc.kill()) }) diff --git a/src/index.spec.ts b/src/index.spec.ts index 0beda2c49..567e84dc0 100644 --- a/src/index.spec.ts +++ b/src/index.spec.ts @@ -1,5 +1,5 @@ import { expect } from 'chai' -import { exec } from 'child_process' +import { exec, spawn } from 'child_process' import { join } from 'path' import semver = require('semver') import ts = require('typescript') @@ -20,6 +20,43 @@ describe('ts-node', function () { }) describe('cli', function () { + this.slow(1000) + + it('should forward signals to the child process', function (done) { + this.slow(5000) + + // We use `spawn` instead because apparently TravisCI + // does not let subprocesses be killed when ran under `sh` + // + // See: https://github.com/travis-ci/travis-ci/issues/704#issuecomment-328278149 + const proc = spawn('node', [ + EXEC_PATH, + '--project', + testDir, + 'tests/signals' + ], { + shell: '/bin/bash' + }) + + let stdout = '' + proc.stdout.on('data', (data) => stdout += data.toString()) + + let stderr = '' + proc.stderr.on('data', (data) => stderr += data.toString()) + + proc.on('exit', function (code) { + expect(stderr).to.equal('') + expect(stdout).to.equal('exited fine') + expect(code).to.equal(0) + + return done() + }) + + // Leave enough time for node to fully start + // the process, then send a signal + setTimeout(() => proc.kill('SIGINT'), 2000) + }) + it('should execute cli', function (done) { exec(`${BIN_EXEC} tests/hello-world`, function (err, stdout) { expect(err).to.equal(null) diff --git a/tests/signals.ts b/tests/signals.ts new file mode 100644 index 000000000..0a0ebc6e7 --- /dev/null +++ b/tests/signals.ts @@ -0,0 +1,12 @@ +process.on('SIGINT', () => { + process.stdout.write('exited') + setTimeout(() => { + process.stdout.write(' fine') + + // Needed to make sure what we wrote has time + // to be written + process.nextTick(() => process.exit()) + }, 500) +}) + +setInterval(() => console.log('should not be reached'), 3000) From 2486ccacd46147d51f704c4af106921cc0cd0b66 Mon Sep 17 00:00:00 2001 From: Blake Embrey Date: Sun, 10 Dec 2017 14:35:57 -0800 Subject: [PATCH 2/3] Update bin.ts --- src/bin.ts | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-) diff --git a/src/bin.ts b/src/bin.ts index a3b864845..d350fd6e5 100644 --- a/src/bin.ts +++ b/src/bin.ts @@ -61,34 +61,24 @@ v8flags(function (err, v8flags) { // // See: https://nodejs.org/api/child_process.html#child_process_options_detached detached: true, - - // Pipe all input and output to this process stdio: 'inherit' } ) - // Ignore signals, and instead forward them to the - // child process - const forward = (signal: NodeJS.Signals) => process.on(signal, () => proc.kill(signal)) - - // Interrupt (CTRL-C) - forward('SIGINT') - - // Termination (`kill` default signal) - forward('SIGTERM') - - // Terminal size change must be forwarded to the subprocess - forward('SIGWINCH') + // Ignore signals, and instead forward them to the child process. + ;['SIGINT', 'SIGTERM', 'SIGWINCH'].forEach(signal => { + process.on(signal, () => proc.kill(signal)) + }) - // On exit, exit this process with the same exit code + // On spawned close, exit this process with the same code. proc.on('close', (code: number, signal: string) => { if (signal) { process.kill(process.pid, signal) - } else if (code) { + } else { process.exit(code) } }) - // If this process is exited, kill the child first - process.on('exit', (_code: number) => proc.kill()) + // If this process exits, kill the child first. + process.on('exit', () => proc.kill()) }) From 986ffa88c167138147321ef77f523d0011de2032 Mon Sep 17 00:00:00 2001 From: Blake Embrey Date: Sun, 10 Dec 2017 14:41:08 -0800 Subject: [PATCH 3/3] Update bin.ts --- src/bin.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/bin.ts b/src/bin.ts index d350fd6e5..697dcbb60 100644 --- a/src/bin.ts +++ b/src/bin.ts @@ -5,6 +5,7 @@ import { join } from 'path' import v8flags = require('v8flags') const argv = process.argv.slice(2) +const signals: NodeJS.Signals[] = ['SIGINT', 'SIGTERM', 'SIGWINCH'] v8flags(function (err, v8flags) { if (err) { @@ -66,9 +67,7 @@ v8flags(function (err, v8flags) { ) // Ignore signals, and instead forward them to the child process. - ;['SIGINT', 'SIGTERM', 'SIGWINCH'].forEach(signal => { - process.on(signal, () => proc.kill(signal)) - }) + signals.forEach(signal => process.on(signal, () => proc.kill(signal))) // On spawned close, exit this process with the same code. proc.on('close', (code: number, signal: string) => {