Skip to content

Commit

Permalink
fix(server): clean up close-server logic (karma-runner#3607)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
devoto13 authored Jan 6, 2021
1 parent 1c9c2de commit 3fca456
Show file tree
Hide file tree
Showing 2 changed files with 93 additions and 154 deletions.
120 changes: 64 additions & 56 deletions lib/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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)
}
}

Expand Down Expand Up @@ -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 = () => {
Expand All @@ -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)
}
})
}
Expand Down Expand Up @@ -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()
})

Expand Down Expand Up @@ -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)
Expand All @@ -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) => {
Expand Down Expand Up @@ -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')
}
Expand Down
Loading

0 comments on commit 3fca456

Please sign in to comment.