From 9f452f6f002c2782c1e175a0075b977a80d27bac Mon Sep 17 00:00:00 2001 From: Yaroslav Admin Date: Thu, 24 Dec 2020 13:02:25 +0200 Subject: [PATCH] fix(server): clean up close server logic 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 | 120 +++++++++++++++++++----------------- test/unit/server.spec.js | 127 +++++++++------------------------------ 2 files changed, 93 insertions(+), 154 deletions(-) diff --git a/lib/server.js b/lib/server.js index 804745088..ff03553eb 100644 --- a/lib/server.js +++ b/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') } diff --git a/test/unit/server.spec.js b/test/unit/server.spec.js index 01121213e..6523a709d 100644 --- a/test/unit/server.spec.js +++ b/test/unit/server.spec.js @@ -15,11 +15,11 @@ 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'. @@ -27,13 +27,11 @@ describe('server', () => { // 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,18 +324,11 @@ 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') @@ -367,18 +336,11 @@ 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('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',