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.

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')
   }