fix(server): clean up close-server logic (#3607)

The main change in behavior is the removal of `dieOnError` method. Previously Karma would send SIGINT to its own process and then trigger clean up logic upon receiving this signal. It is a pretty convoluted way to trigger shutdown. This commit extracts clean up logic into the `_close()` method and calls this method directly everywhere.

This change solves two issues:
- Makes life easier for other tools (like Angular CLI), which use Karma programmatically from another process and killing whole process on Karma error may not be the most convenient behavior. Instead Karma will clean up all its resources and notify caller using the `done` callback.
- Allows to remove last Grunt bits in the future PR. When running unit tests without Grunt wrapper the SIGINT is received by the Mocha process, which stops tests execution midway.

Author: devoto13

lib/server.js

@@ -102,12 +102,6 @@ class Server extends KarmaEventEmitter {
     this._injector = new di.Injector(modules)
   }
 
-  dieOnError (error) {
-    this.log.error(error)
-    process.exitCode = 1
-    process.kill(process.pid, 'SIGINT')
-  }
-
   async start () {
     const config = this.get('config')
     try {
@@ -122,7 +116,8 @@ class Server extends KarmaEventEmitter {
       config.port = this._boundServer.address().port
       await this._injector.invoke(this._start, this)
     } catch (err) {
-      this.dieOnError(`Server start failed on port ${config.port}: ${err}`)
+      this.log.error(`Server start failed on port ${config.port}: ${err}`)
+      this._close(1)
     }
   }
 
@@ -187,7 +182,8 @@ class Server extends KarmaEventEmitter {
     let singleRunBrowserNotCaptured = false
 
     webServer.on('error', (err) => {
-      this.dieOnError(`Webserver fail ${err}`)
+      this.log.error(`Webserver fail ${err}`)
+      this._close(1)
     })
 
     const afterPreprocess = () => {
@@ -206,7 +202,8 @@ class Server extends KarmaEventEmitter {
           })
         }
         if (this.loadErrors.length > 0) {
-          this.dieOnError(new Error(`Found ${this.loadErrors.length} load error${this.loadErrors.length === 1 ? '' : 's'}`))
+          this.log.error(new Error(`Found ${this.loadErrors.length} load error${this.loadErrors.length === 1 ? '' : 's'}`))
+          this._close(1)
         }
       })
     }
@@ -302,9 +299,9 @@ class Server extends KarmaEventEmitter {
       }
     })
 
-    this.on('stop', function (done) {
+    this.on('stop', (done) => {
       this.log.debug('Received stop event, exiting.')
-      disconnectBrowsers()
+      this._close()
       done()
     })
 
@@ -332,9 +329,9 @@ class Server extends KarmaEventEmitter {
         emitRunCompleteIfAllBrowsersDone()
       })
 
-      this.on('run_complete', function (browsers, results) {
+      this.on('run_complete', (browsers, results) => {
         this.log.debug('Run complete, exiting.')
-        disconnectBrowsers(results.exitCode)
+        this._close(results.exitCode)
       })
 
       this.emit('run_start', singleRunBrowsers)
@@ -350,52 +347,13 @@ class Server extends KarmaEventEmitter {
       })
     }
 
-    const webServerCloseTimeout = 3000
-    const disconnectBrowsers = (code) => {
-      const sockets = socketServer.sockets.sockets
-
-      Object.keys(sockets).forEach((id) => {
-        const socket = sockets[id]
-        socket.removeAllListeners('disconnect')
-        if (!socket.disconnected) {
-          process.nextTick(socket.disconnect.bind(socket))
-        }
-      })
-
-      this.emitExitAsync(code).catch((err) => {
-        this.log.error('Error while calling exit event listeners\n' + err.stack || err)
-        return 1
-      }).then((code) => {
-        socketServer.sockets.removeAllListeners()
-        socketServer.close()
-
-        let removeAllListenersDone = false
-        const removeAllListeners = () => {
-          if (removeAllListenersDone) {
-            return
-          }
-          removeAllListenersDone = true
-          webServer.removeAllListeners()
-          processWrapper.removeAllListeners()
-          done(code || 0)
-        }
-
-        const closeTimeout = setTimeout(removeAllListeners, webServerCloseTimeout)
-
-        webServer.close(() => {
-          clearTimeout(closeTimeout)
-          removeAllListeners()
-        })
-      })
-    }
-
-    processWrapper.on('SIGINT', () => disconnectBrowsers(process.exitCode))
-    processWrapper.on('SIGTERM', disconnectBrowsers)
+    processWrapper.on('SIGINT', () => this._close())
+    processWrapper.on('SIGTERM', () => this._close())
 
     const reportError = (error) => {
-      process.emit('infrastructure_error', error)
-      disconnectBrowsers(1)
       this.log.error(error)
+      process.emit('infrastructure_error', error)
+      this._close(1)
     }
 
     processWrapper.on('unhandledRejection', (error) => {
@@ -429,6 +387,56 @@ class Server extends KarmaEventEmitter {
     child.unref()
   }
 
+  /**
+   * Cleanup all resources allocated by Karma and call the `done` callback
+   * with the result of the tests execution.
+   *
+   * @param [exitCode] - Optional exit code. If omitted will be computed by
+   * 'exit' event listeners.
+   */
+  _close (exitCode) {
+    const webServer = this._injector.get('webServer')
+    const socketServer = this._injector.get('socketServer')
+    const done = this._injector.get('done')
+
+    const webServerCloseTimeout = 3000
+    const sockets = socketServer.sockets.sockets
+
+    Object.keys(sockets).forEach((id) => {
+      const socket = sockets[id]
+      socket.removeAllListeners('disconnect')
+      if (!socket.disconnected) {
+        process.nextTick(socket.disconnect.bind(socket))
+      }
+    })
+
+    this.emitExitAsync(exitCode).catch((err) => {
+      this.log.error('Error while calling exit event listeners\n' + err.stack || err)
+      return 1
+    }).then((code) => {
+      socketServer.sockets.removeAllListeners()
+      socketServer.close()
+
+      let removeAllListenersDone = false
+      const removeAllListeners = () => {
+        if (removeAllListenersDone) {
+          return
+        }
+        removeAllListenersDone = true
+        webServer.removeAllListeners()
+        processWrapper.removeAllListeners()
+        done(code || 0)
+      }
+
+      const closeTimeout = setTimeout(removeAllListeners, webServerCloseTimeout)
+
+      webServer.close(() => {
+        clearTimeout(closeTimeout)
+        removeAllListeners()
+      })
+    })
+  }
+
   stop () {
     return this.emitAsync('stop')
   }

test/unit/server.spec.js

@@ -15,25 +15,23 @@ describe('server', () => {
   let mockSocketServer
   let mockBoundServer
   let mockExecutor
-  let doneSpy
+  let doneStub
   let logErrorSpy
   let server = mockConfig = browserCollection = webServerOnError = null
   let fileListOnResolve = fileListOnReject = mockLauncher = null
-  let mockFileList = mockWebServer = mockSocketServer = mockExecutor = doneSpy = null
+  let mockFileList = mockWebServer = mockSocketServer = mockExecutor = doneStub = null
   const mockSocketEventListeners = new Map()
 
   // Use regular function not arrow so 'this' is mocha 'this'.
   beforeEach(function () {
     // The first call to new Server() loads plugins and it can take >2000ms.
     this.timeout(4000)
     browserCollection = new BrowserCollection()
-    doneSpy = sinon.spy()
+    doneStub = sinon.stub()
     logErrorSpy = sinon.spy(logger.create('karma-server'), 'error')
 
     fileListOnResolve = fileListOnReject = null
 
-    doneSpy = sinon.spy()
-
     mockConfig = {
       frameworks: [],
       port: 9876,
@@ -49,7 +47,7 @@ describe('server', () => {
       browserNoActivityTimeout: 0
     }
 
-    server = new Server(mockConfig, doneSpy)
+    server = new Server(mockConfig, doneStub)
 
     sinon.stub(server._injector, 'invoke').returns([])
 
@@ -120,10 +118,10 @@ describe('server', () => {
       close: sinon.spy((callback) => callback && callback())
     }
 
-    sinon
-      .stub(server._injector, 'get')
-      .withArgs('webServer').returns(mockWebServer)
-      .withArgs('socketServer').returns(mockSocketServer)
+    const injectorStub = sinon.stub(server._injector, 'get')
+    injectorStub.withArgs('socketServer').returns(mockSocketServer)
+    injectorStub.withArgs('webServer').returns(mockWebServer)
+    injectorStub.callThrough()
 
     webServerOnError = null
   })
@@ -185,7 +183,7 @@ describe('server', () => {
     })
 
     it('should start the web server after all files have been preprocessed successfully', async () => {
-      await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneSpy)
+      await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneStub)
 
       expect(mockFileList.refresh).to.have.been.called
       expect(fileListOnResolve).not.to.be.null
@@ -199,7 +197,7 @@ describe('server', () => {
     })
 
     it('should start the web server after all files have been preprocessed with an error', async () => {
-      await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneSpy)
+      await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneStub)
 
       expect(mockFileList.refresh).to.have.been.called
       expect(fileListOnReject).not.to.be.null
@@ -215,7 +213,7 @@ describe('server', () => {
     })
 
     it('should launch browsers after the web server has started', async () => {
-      await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneSpy)
+      await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneStub)
 
       expect(mockWebServer.listen).not.to.have.been.called
       expect(webServerOnError).not.to.be.null
@@ -227,7 +225,7 @@ describe('server', () => {
     })
 
     it('should emit a listening event once server begin accepting connections', async () => {
-      await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneSpy)
+      await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneStub)
 
       const listening = sinon.spy()
       server.on('listening', listening)
@@ -239,7 +237,7 @@ describe('server', () => {
     })
 
     it('should emit a browsers_ready event once all the browsers are captured', async () => {
-      await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneSpy)
+      await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneStub)
 
       const browsersReady = sinon.spy()
       server.on('browsers_ready', browsersReady)
@@ -254,7 +252,7 @@ describe('server', () => {
     })
 
     it('should emit a browser_register event for each browser added', async () => {
-      await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneSpy)
+      await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneStub)
 
       const browsersReady = sinon.spy()
       server.on('browsers_ready', browsersReady)
@@ -276,43 +274,28 @@ describe('server', () => {
         })
       }
 
-      it('1 on load errors', async () => {
-        mockProcess(process)
+      beforeEach(() => {
+        doneStub.callsFake((exitCode) => resolveExitCode(exitCode))
+      })
 
-        await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, (exitCode) => {
-          resolveExitCode(exitCode)
-        })
+      it('1 on load errors', async () => {
+        await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneStub)
         server.loadErrors.push(['TestError', 'Test'])
         fileListOnResolve()
 
-        function mockProcess (process) {
-          sinon.stub(process, 'kill').callsFake((pid, ev) => process.emit(ev))
-        }
-
         expect(await exitCode()).to.have.equal(1)
       })
 
       it('given on run_complete', async () => {
-        mockProcess(process)
-
-        await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, (exitCode) => {
-          resolveExitCode(exitCode)
-        })
+        await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneStub)
 
         server.emit('run_complete', browserCollection, { exitCode: 15 })
 
-        function mockProcess (process) {
-          sinon.stub(process, 'kill').callsFake((pid, ev) => process.emit(ev))
-        }
         expect(await exitCode()).to.have.equal(15)
       })
 
       it('given on run_complete with exit event listener (15)', async () => {
-        mockProcess(process)
-
-        await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, (exitCode) => {
-          resolveExitCode(exitCode)
-        })
+        await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneStub)
 
         // last non-zero exit code will be taken
         server.on('exit', (done) => {
@@ -328,18 +311,11 @@ describe('server', () => {
         // Provided run_complete exitCode will be overridden by exit listeners
         server.emit('run_complete', browserCollection, { exitCode: 5 })
 
-        function mockProcess (process) {
-          sinon.stub(process, 'kill').callsFake((pid, ev) => process.emit(ev))
-        }
         expect(await exitCode()).to.have.equal(15)
       })
 
       it('given on run_complete with exit event listener (0)', async () => {
-        mockProcess(process)
-
-        await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, (exitCode) => {
-          resolveExitCode(exitCode)
-        })
+        await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneStub)
 
         // exit listeners can't set exit code back to 0
         server.on('exit', (done) => {
@@ -348,37 +324,23 @@ describe('server', () => {
 
         server.emit('run_complete', browserCollection, { exitCode: 15 })
 
-        function mockProcess (process) {
-          sinon.stub(process, 'kill').callsFake((pid, ev) => process.emit(ev))
-        }
         expect(await exitCode()).to.have.equal(15)
       })
 
       it('1 on run_complete with exit event listener throws', async () => {
-        mockProcess(process)
-
-        await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, (exitCode) => {
-          resolveExitCode(exitCode)
-        })
+        await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneStub)
 
         server.on('exit', (done) => {
           throw new Error('async error from exit event listener')
         })
 
         server.emit('run_complete', browserCollection, { exitCode: 0 })
 
-        function mockProcess (process) {
-          sinon.stub(process, 'kill').callsFake((pid, ev) => process.emit(ev))
-        }
         expect(await exitCode()).to.have.equal(1)
       })
 
       it('1 on run_complete with exit event listener rejects', async () => {
-        mockProcess(process)
-
-        await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, (exitCode) => {
-          resolveExitCode(exitCode)
-        })
+        await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneStub)
 
         function onExit (done) {
           // Need to remove listener to prevent endless loop via unhandledRejection handler
@@ -390,71 +352,40 @@ describe('server', () => {
 
         server.emit('run_complete', browserCollection, { exitCode: 0 })
 
-        function mockProcess (process) {
-          sinon.stub(process, 'kill').callsFake((pid, ev) => process.emit(ev))
-        }
         expect(await exitCode()).to.have.equal(1)
       })
 
       it('0 on server stop', async () => {
-        mockProcess(process)
-
-        await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, (exitCode) => {
-          resolveExitCode(exitCode)
-        })
+        await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneStub)
 
         server.stop()
 
-        function mockProcess (process) {
-          sinon.stub(process, 'kill').callsFake((pid, ev) => process.emit(ev))
-        }
         expect(await exitCode()).to.have.equal(0)
       })
 
       it('1 on browser_process_failure (singleRunBrowserNotCaptured)', async () => {
-        mockProcess(process)
-
-        await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, (exitCode) => {
-          resolveExitCode(exitCode)
-        })
+        await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneStub)
 
         server.emit('browser_process_failure', { id: 'fake' })
 
-        function mockProcess (process) {
-          sinon.stub(process, 'kill').callsFake((pid, ev) => process.emit(ev))
-        }
         expect(await exitCode()).to.have.equal(1)
       })
 
       it('0 on browser_complete_with_no_more_retries', async () => {
-        mockProcess(process)
-
-        await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, (exitCode) => {
-          resolveExitCode(exitCode)
-        })
+        await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneStub)
 
         server.emit('browser_complete_with_no_more_retries', { id: 'fake', remove: () => {} })
 
-        function mockProcess (process) {
-          sinon.stub(process, 'kill').callsFake((pid, ev) => process.emit(ev))
-        }
         expect(await exitCode()).to.have.equal(0)
       })
 
       it('1 on browser_complete_with_no_more_retries with config.failOnEmptyTestSuite', async () => {
-        mockProcess(process)
-
         mockConfig.failOnEmptyTestSuite = true
 
-        await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, (exitCode) => {
-          resolveExitCode(exitCode)
-        })
+        await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneStub)
 
         server.emit('browser_complete_with_no_more_retries', { id: 'fake', remove: () => {} })
 
-        function mockProcess (process) {
-          sinon.stub(process, 'kill').callsFake((pid, ev) => process.emit(ev))
-        }
         expect(await exitCode()).to.have.equal(1)
       })
     })
@@ -465,7 +396,7 @@ describe('server', () => {
 
     beforeEach(async () => {
       browserCollection = new BrowserCollection(server)
-      await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneSpy)
+      await server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneStub)
 
       mockBrowserSocket = {
         id: 'browser-socket-id',