From 1777152f8de4c804ce2d559bda77b686c53f7445 Mon Sep 17 00:00:00 2001 From: Sergei Startsev Date: Fri, 29 Jun 2018 19:48:24 +0300 Subject: [PATCH] fix(server): pass handle to webServer to prevent`EADDRINUSE` issue for tests running in parallel Fixed #3065 --- lib/server.js | 7 +++- lib/utils/net-utils.js | 37 +++++++++--------- package.json | 1 + test/unit/server.spec.js | 63 ++++++++++++++++++++----------- test/unit/utils/net-utils.spec.js | 55 +++++++++++---------------- 5 files changed, 85 insertions(+), 78 deletions(-) diff --git a/lib/server.js b/lib/server.js index cbad577ed..f30510242 100644 --- a/lib/server.js +++ b/lib/server.js @@ -117,8 +117,11 @@ class Server extends KarmaEventEmitter { BundleUtils.bundleResourceIfNotExist('context/main.js', 'static/context.js') ]) .then(() => NetUtils.getAvailablePort(config.port, config.listenAddress)) - .then((port) => { + .then((bound) => { + const port = bound[0] + const server = bound[1] config.port = port + this._boundServer = server this._injector.invoke(this._start, this) }) .catch(this.dieOnError.bind(this)) @@ -156,7 +159,7 @@ class Server extends KarmaEventEmitter { this._injector.invoke(watcher.watch) } - webServer.listen(config.port, config.listenAddress, () => { + webServer.listen(this._boundServer, () => { this.log.info(`Karma v${constant.VERSION} server started at ${config.protocol}//${config.listenAddress}:${config.port}${config.urlRoot}`) this.emit('listening', config.port) diff --git a/lib/utils/net-utils.js b/lib/utils/net-utils.js index 7262ba40b..04b431e28 100644 --- a/lib/utils/net-utils.js +++ b/lib/utils/net-utils.js @@ -4,29 +4,26 @@ const Promise = require('bluebird') const net = require('net') const NetUtils = { - isPortAvailable (port, listenAddress) { - return new Promise((resolve, reject) => { - const server = net.createServer() + getAvailablePort (port, listenAddress) { + function listen (port, listenAddress) { + return new Promise((resolve, reject) => { + const server = net.createServer() - server.unref() - server.on('error', (err) => { - server.close() - if (err.code === 'EADDRINUSE' || err.code === 'EACCES') { - resolve(false) - } else { - reject(err) - } - }) + server.on('error', (err) => { + server.close() + if (err.code === 'EADDRINUSE' || err.code === 'EACCES') { + server.listen(++port, listenAddress) + } else { + reject(err) + } + }) - server.listen(port, listenAddress, () => { - server.close(() => resolve(true)) + server.listen(port, listenAddress, () => { + resolve([port, server]) + }) }) - }) - }, - - getAvailablePort (port, listenAddress) { - return NetUtils.isPortAvailable(port, listenAddress) - .then((available) => available ? port : NetUtils.getAvailablePort(port + 1, listenAddress)) + } + return listen(port, listenAddress) } } diff --git a/package.json b/package.json index e111f79b3..6bac5fdf5 100644 --- a/package.json +++ b/package.json @@ -195,6 +195,7 @@ "Samuel Marks ", "Saugat Acharya ", "Schmulik Raskin ", + "Sergei Startsev ", "Sergey Kruk ", "Sergey Simonchik ", "Seth Rhodes ", diff --git a/test/unit/server.spec.js b/test/unit/server.spec.js index a13104739..4012d8f6b 100644 --- a/test/unit/server.spec.js +++ b/test/unit/server.spec.js @@ -11,6 +11,7 @@ describe('server', () => { var mockLauncher var mockWebServer var mockSocketServer + var mockBoundServer var mockExecutor var doneSpy var server = mockConfig = browserCollection = webServerOnError = null @@ -45,8 +46,7 @@ describe('server', () => { sinon.stub(server._injector, 'invoke').returns([]) - mockExecutor = - {schedule: () => {}} + mockExecutor = { schedule: () => {} } mockFileList = { refresh: sinon.spy(() => { @@ -80,6 +80,12 @@ describe('server', () => { } } + mockBoundServer = { + on: sinon.spy((event, callback) => callback && callback()), + listen: sinon.spy((port, listenAddress, callback) => callback && callback()), + close: sinon.spy((callback) => callback && callback()) + } + mockWebServer = { on (name, handler) { if (name === 'error') { @@ -99,18 +105,26 @@ describe('server', () => { close: sinon.spy((callback) => callback && callback()) } - sinon.stub(server._injector, 'get') - .withArgs('webServer').returns(mockWebServer) - .withArgs('socketServer').returns(mockSocketServer) + sinon + .stub(server._injector, 'get') + .withArgs('webServer') + .returns(mockWebServer) + .withArgs('socketServer') + .returns(mockSocketServer) webServerOnError = null }) describe('start', () => { + var config beforeEach(() => { + config = { port: 9876, listenAddress: '127.0.0.1' } sinon.spy(BundleUtils, 'bundleResourceIfNotExist') - sinon.stub(NetUtils, 'getAvailablePort').resolves(9876) - sinon.stub(server, 'get').withArgs('config').returns({ port: 9876, listenAddress: '127.0.0.1' }) + sinon.stub(NetUtils, 'getAvailablePort').resolves([9877, mockBoundServer]) + sinon + .stub(server, 'get') + .withArgs('config') + .returns(config) }) it('should compile static resources', (done) => { @@ -127,12 +141,25 @@ describe('server', () => { done() }) }) + + it('should change config.port to available', (done) => { + expect(config.port).to.be.equal(9876) + server.start().then(() => { + expect(config.port).to.be.equal(9877) + expect(server._boundServer).to.be.equal(mockBoundServer) + done() + }) + }) }) // ============================================================================ // server._start() // ============================================================================ describe('_start', () => { + beforeEach(() => { + server._boundServer = mockBoundServer + }) + it('should start the web server after all files have been preprocessed successfully', () => { server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneSpy) @@ -142,7 +169,8 @@ describe('server', () => { expect(server._injector.invoke).not.to.have.been.calledWith(mockLauncher.launch, mockLauncher) fileListOnResolve() - expect(mockWebServer.listen).to.have.been.called + expect(mockWebServer.listen).to.have.been.calledWith(mockBoundServer, sinon.match.func) + expect(webServerOnError).not.to.be.null expect(server._injector.invoke).to.have.been.calledWith(mockLauncher.launch, mockLauncher) }) @@ -155,7 +183,8 @@ describe('server', () => { expect(server._injector.invoke).not.to.have.been.calledWith(mockLauncher.launch, mockLauncher) fileListOnReject() - expect(mockWebServer.listen).to.have.been.called + expect(mockWebServer.listen).to.have.been.calledWith(mockBoundServer, sinon.match.func) + expect(webServerOnError).not.to.be.null expect(server._injector.invoke).to.have.been.calledWith(mockLauncher.launch, mockLauncher) }) @@ -163,26 +192,14 @@ describe('server', () => { server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneSpy) expect(mockWebServer.listen).not.to.have.been.called + expect(webServerOnError).not.to.be.null expect(server._injector.invoke).not.to.have.been.calledWith(mockLauncher.launch, mockLauncher) fileListOnResolve() - expect(mockWebServer.listen).to.have.been.called + expect(mockWebServer.listen).to.have.been.calledWith(mockBoundServer, sinon.match.func) expect(server._injector.invoke).to.have.been.calledWith(mockLauncher.launch, mockLauncher) }) - it('should listen on the listenAddress in the config', () => { - server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneSpy) - - expect(mockWebServer.listen).not.to.have.been.called - expect(webServerOnError).not.to.be.null - - expect(mockConfig.listenAddress).to.be.equal('127.0.0.1') - - fileListOnResolve() - expect(mockWebServer.listen).to.have.been.calledWith(9876, '127.0.0.1') - expect(mockConfig.listenAddress).to.be.equal('127.0.0.1') - }) - it('should emit a listening event once server begin accepting connections', () => { server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneSpy) diff --git a/test/unit/utils/net-utils.spec.js b/test/unit/utils/net-utils.spec.js index 23d32e4cf..dc685e076 100644 --- a/test/unit/utils/net-utils.spec.js +++ b/test/unit/utils/net-utils.spec.js @@ -4,46 +4,35 @@ const NetUtils = require('../../../lib/utils/net-utils') const connect = require('connect') const net = require('net') -describe('NetUtils.isPortAvailable', () => { - it('it is possible to run server on available port', (done) => { - NetUtils.isPortAvailable(9876, '127.0.0.1').then((available) => { - expect(available).to.be.true - const server = net - .createServer(connect()) - .listen(9876, '127.0.0.1', () => { - server.close(done) - }) +describe('NetUtils.getAvailablePort', () => { + it('resolves server with bound port when it is available', (done) => { + NetUtils.getAvailablePort(9876, '127.0.0.1').then((bound) => { + const port = bound[0] + const boundServer = bound[1] + expect(port).to.be.equal(9876) + expect(boundServer).not.to.be.null + const server = net.createServer(connect()).listen(boundServer, () => { + server.close(done) + }) }) }) - it('resolves with false when port is used', (done) => { - const server = net - .createServer(connect()) - .listen(9876, '127.0.0.1', () => { - NetUtils.isPortAvailable(9876, '127.0.0.1').then((available) => { - expect(available).to.be.false - server.close(done) - }) + it('resolves with next available port', (done) => { + const server = net.createServer(connect()).listen(9876, '127.0.0.1', () => { + NetUtils.getAvailablePort(9876, '127.0.0.1').then((bound) => { + const port = bound[0] + const boundServer = bound[1] + expect(port).to.be.equal(9877) + expect(boundServer).not.to.be.null + server.close(done) }) - }) -}) - -describe('NetUtils.getAvailablePort', () => { - it('resolves with port when is available', (done) => { - NetUtils.getAvailablePort(9876, '127.0.0.1').then((port) => { - expect(port).to.equal(9876) - done() }) }) - it('resolves with next available port', (done) => { - const stub = sinon.stub(NetUtils, 'isPortAvailable') - stub.withArgs(9876).resolves(false) - stub.withArgs(9877).resolves(false) - stub.withArgs(9878).resolves(true) - - NetUtils.getAvailablePort(9876, '127.0.0.1').then((port) => { - expect(port).to.equal(9878) + it('rejects if a critical error occurs', (done) => { + const incorrectAddress = '123.321' + NetUtils.getAvailablePort(9876, incorrectAddress).catch((err) => { + expect(err).not.to.be.null done() }) })