Fix bugs in parent and spawn process termination (#36)

* Fix spawn and parent process killing
- Implemented a broader solution to killing the parent/spawned process
that includes hooks for certain specific signals and then the generic
node exit event
- Added test cases to support the new termination solution

* Updated a bunch of node dependencies

* Updated change log

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## 8.0.0 - In Development
 - ***BREAKING***: Stripe out spaces around the `key` and `value` in an env file. In order to include a beginning/ending space in an env var value, you need to surround the value in double or single quotes. `ENV = " Value"`
+- **Bug**: Fixed some bugs around how the parent process and spawn processes are killed
+- **Change**: Updated a number of core libraries: `cross-spawn`, `coveralls`, `istanbul -> nyc`, `mocha`, `proxyquire`, `sinon`, `standard`
 
 ## 7.0.0
 - ***BREAKING***: The `.env` file path resolving has been changed to allow for absolute pathing, relative pathing, and `~` home directory pathing. Please

lib/index.js

@@ -6,6 +6,9 @@ const fs = require('fs')
 const os = require('os')
 const rcFileLocation = path.join(process.cwd(), '.env-cmdrc')
 const envFilePathDefault = path.join(process.cwd(), '.env')
+let sharedState = {
+  exitCalled: false
+}
 
 /**
  * The main process for reading, parsing, applying and then running the process with env vars
@@ -40,8 +43,14 @@ function EnvCmd (args) {
     stdio: 'inherit',
     env
   })
-  process.on('SIGTERM', proc.kill.bind(proc, 'SIGTERM'))
-  proc.on('exit', process.exit)
+
+  // Handle a few special signals and then the general node exit event
+  // on both parent and spawned process
+  process.once('SIGINT', TerminateSpawnedProc.bind(sharedState, proc))
+  process.once('SIGTERM', TerminateSpawnedProc.bind(sharedState, proc))
+  process.once('exit', TerminateSpawnedProc.bind(sharedState, proc))
+  proc.on('exit', TerminateParentProcess)
+
   return proc
 }
 
@@ -293,6 +302,27 @@ function ResolveEnvFilePath (userPath) {
   return path.resolve(process.cwd(), userPath)
 }
 
+/**
+ * Helper for terminating the spawned process
+ * @param       {ProccessHandler} proc The spawned process handler
+ */
+function TerminateSpawnedProc (proc) {
+  if (!this.exitCalled) {
+    this.exitCalled = true
+    proc.kill('SIGTERM')
+  }
+}
+
+/**
+ * Helper for terminating the parent process
+ */
+function TerminateParentProcess () {
+  if (!this.exitCalled) {
+    this.exitCalled = true
+    process.exit(1)
+  }
+}
+
 process.on('uncaughtException', HandleUncaughtExceptions)
 
 module.exports = {
@@ -301,6 +331,8 @@ module.exports = {
   ParseEnvString,
   PrintHelp,
   HandleUncaughtExceptions,
+  TerminateSpawnedProc,
+  TerminateParentProcess,
   StripComments,
   StripEmptyLines,
   ParseEnvVars,

package.json

@@ -3,15 +3,15 @@
   "version": "7.0.0",
   "description": "Executes a command using the envs in the provided env file",
   "main": "lib/index.js",
-  "engines" : {
-    "node" : ">=4.0.0"
+  "engines": {
+    "node": ">=4.0.0"
   },
   "bin": {
     "env-cmd": "bin/env-cmd.js"
   },
   "scripts": {
     "test": "mocha",
-    "test-cover": "istanbul cover node_modules/.bin/_mocha -- -R spec",
+    "test-cover": "nyc --reporter=lcov --reporter=text npm test",
     "test-lint": "standard",
     "coveralls": "coveralls < coverage/lcov.info",
     "lint": "standard --fix"
@@ -41,15 +41,15 @@
   },
   "homepage": "https://github.com/toddbluhm/env-cmd#readme",
   "dependencies": {
-    "cross-spawn": "^5.0.1"
+    "cross-spawn": "^6.0.5"
   },
   "devDependencies": {
     "better-assert": "^1.0.2",
-    "coveralls": "^2.11.12",
-    "istanbul": "^0.4.4",
-    "mocha": "^3.0.2",
-    "proxyquire": "^1.7.10",
-    "sinon": "^3.3.0",
-    "standard": "^10.0.0"
+    "coveralls": "^3.0.0",
+    "mocha": "^5.0.4",
+    "nyc": "^11.6.0",
+    "proxyquire": "^2.0.1",
+    "sinon": "^4.4.6",
+    "standard": "^11.0.1"
   }
 }

test/test.js

@@ -46,6 +46,8 @@ const StripComments = lib.StripComments
 const StripEmptyLines = lib.StripEmptyLines
 const ParseEnvVars = lib.ParseEnvVars
 const ResolveEnvFilePath = lib.ResolveEnvFilePath
+const TerminateSpawnedProc = lib.TerminateSpawnedProc
+const TerminateParentProcess = lib.TerminateParentProcess
 
 describe('env-cmd', function () {
   describe('ParseArgs', function () {
@@ -181,7 +183,7 @@ describe('env-cmd', function () {
       proxyquire.callThru()
     })
     afterEach(function () {
-      spawnStub.reset()
+      spawnStub.resetHistory()
     })
     it('should parse env vars from JSON with node module loader if file extension is .json', function () {
       EnvCmd(['./test/.env.json', 'echo', '$BOB'])
@@ -227,7 +229,7 @@ describe('env-cmd', function () {
       proxyquire.callThru()
     })
     afterEach(function () {
-      spawnStub.reset()
+      spawnStub.resetHistory()
     })
     it('should parse env vars from .env-cmdrc file using development env', function () {
       EnvCmd(['development', 'echo', '$BOB'])
@@ -318,8 +320,9 @@ describe('env-cmd', function () {
       this.readFileStub.restore()
     })
     afterEach(function () {
-      spawnStub.reset()
-      this.readFileStub.reset()
+      spawnStub.resetHistory()
+      this.readFileStub.resetHistory()
+      process.removeAllListeners();
     })
     it('should spawn a new process with the env vars set', function () {
       this.readFileStub.returns('BOB=COOL\nNODE_ENV=dev\nANSWER=42\n')
@@ -396,13 +399,13 @@ describe('env-cmd', function () {
     it('should print help text and error if error contains \'passed\'', function () {
       HandleUncaughtExceptions(new Error('print help text passed now'))
       assert(this.logStub.calledTwice)
-      this.logStub.restore()  // restore here so test success logs get printed
+      this.logStub.restore() // restore here so test success logs get printed
     })
 
     it('should print just there error if error does not contain \'passed\'', function () {
       HandleUncaughtExceptions(new Error('do not print help text now'))
       assert(this.logStub.calledOnce)
-      this.logStub.restore()  // restore here so test success logs get printed
+      this.logStub.restore() // restore here so test success logs get printed
     })
   })
 
@@ -444,4 +447,59 @@ describe('env-cmd', function () {
       assert(abPath === '/Users/hitchhikers-guide-to-the-galaxy/Thanks/~/fish.env')
     })
   })
+
+  describe('TerminateSpawnedProc', function () {
+    beforeEach(function () {
+      this.procStub = sinon.stub()
+      this.proc = {
+        kill: this.procStub
+      }
+      this.exitCalled = false
+    })
+
+    it('should call kill method on spawned process', function () {
+      TerminateSpawnedProc.call(this, this.proc)
+      assert(this.procStub.callCount === 1)
+    })
+
+    it('should not call kill method more than once', function () {
+      TerminateSpawnedProc.call(this, this.proc)
+      TerminateSpawnedProc.call(this, this.proc)
+      assert(this.procStub.callCount === 1)
+    })
+
+    it('should not call kill method if the spawn process is already dying', function () {
+      this.exitCalled = true
+      TerminateSpawnedProc.call(this, this.proc)
+      assert(this.procStub.callCount === 0)
+    })
+  })
+
+  describe('TerminateParentProcess', function () {
+    beforeEach(function () {
+      this.exitStub = sinon.stub(process, 'exit')
+      this.exitCalled = false
+    })
+
+    afterEach(function () {
+      this.exitStub.restore()
+    })
+
+    it('should call exit method on parent process', function () {
+      TerminateParentProcess.call(this)
+      assert(this.exitStub.callCount === 1)
+    })
+
+    it('should not call exit method more than once', function () {
+      TerminateParentProcess.call(this)
+      TerminateParentProcess.call(this)
+      assert(this.exitStub.callCount === 1)
+    })
+
+    it('should not call exit method if the process is already dying', function () {
+      this.exitCalled = true
+      TerminateParentProcess.call(this)
+      assert(this.exitStub.callCount === 0)
+    })
+  })
 })