Skip to content

Commit

Permalink
fix(server): pass bound port to preventEADDRINUSE issue. (#3065)
Browse files Browse the repository at this point in the history
  • Loading branch information
@sergei-startsev
sergei-startsev committed Jul 1, 2018
1 parent ad820a1 commit 0f78818
Show file tree
Hide file tree
Showing 5 changed files with 84 additions and 79 deletions.
9 changes: 6 additions & 3 deletions lib/server.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,9 +116,12 @@ class Server extends KarmaEventEmitter {
BundleUtils.bundleResourceIfNotExist('client/main.js', 'static/karma.js'),
BundleUtils.bundleResourceIfNotExist('context/main.js', 'static/context.js')
])
.then(() => NetUtils.getAvailablePort(config.port, config.listenAddress))
.then((port) => {
.then(() => NetUtils.findAndBindAvailablePort(config.port, config.listenAddress))
.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))
Expand Down Expand Up @@ -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)
Expand Down
33 changes: 14 additions & 19 deletions lib/utils/net-utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,29 +4,24 @@ const Promise = require('bluebird')
const net = require('net')

const NetUtils = {
isPortAvailable (port, listenAddress) {
findAndBindAvailablePort (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.listen(port, listenAddress, () => {
server.close(() => resolve(true))
})
server
.on('error', (err) => {
server.close()
if (err.code === 'EADDRINUSE' || err.code === 'EACCES') {
server.listen(++port, listenAddress)
} else {
reject(err)
}
})
.on('listening', () => {
resolve([port, server])
})
.listen(port, listenAddress)
})
},

getAvailablePort (port, listenAddress) {
return NetUtils.isPortAvailable(port, listenAddress)
.then((available) => available ? port : NetUtils.getAvailablePort(port + 1, listenAddress))
}
}

Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,7 @@
"Samuel Marks <[email protected]>",
"Saugat Acharya <[email protected]>",
"Schmulik Raskin <[email protected]>",
"Sergei Startsev <[email protected]>",
"Sergey Kruk <[email protected]>",
"Sergey Simonchik <[email protected]>",
"Seth Rhodes <[email protected]>",
Expand Down
65 changes: 41 additions & 24 deletions test/unit/server.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ describe('server', () => {
var mockLauncher
var mockWebServer
var mockSocketServer
var mockBoundServer
var mockExecutor
var doneSpy
var server = mockConfig = browserCollection = webServerOnError = null
Expand Down Expand Up @@ -45,8 +46,7 @@ describe('server', () => {

sinon.stub(server._injector, 'invoke').returns([])

mockExecutor =
{schedule: () => {}}
mockExecutor = { schedule: () => {} }

mockFileList = {
refresh: sinon.spy(() => {
Expand Down Expand Up @@ -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') {
Expand All @@ -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, 'findAndBindAvailablePort').resolves([9877, mockBoundServer])
sinon
.stub(server, 'get')
.withArgs('config')
.returns(config)
})

it('should compile static resources', (done) => {
Expand All @@ -123,7 +137,16 @@ describe('server', () => {

it('should search for available port', (done) => {
server.start().then(() => {
expect(NetUtils.getAvailablePort).to.have.been.calledWith(9876, '127.0.0.1')
expect(NetUtils.findAndBindAvailablePort).to.have.been.calledWith(9876, '127.0.0.1')
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()
})
})
Expand All @@ -133,6 +156,10 @@ describe('server', () => {
// 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)

Expand All @@ -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)
})

Expand All @@ -155,34 +183,23 @@ 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)
})

it('should launch browsers after the web server has started', () => {
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)

Expand Down
55 changes: 22 additions & 33 deletions test/unit/utils/net-utils.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.findAndBindAvailablePort', () => {
it('resolves server with bound port when it is available', (done) => {
NetUtils.findAndBindAvailablePort(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.findAndBindAvailablePort(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.findAndBindAvailablePort(9876, incorrectAddress).catch((err) => {
expect(err).not.to.be.null
done()
})
})
Expand Down

0 comments on commit 0f78818

Please sign in to comment.