fix(server): pass bound port to preventEADDRINUSE issue. (#3065)

sergei-startsev · lusarz · commit 850a90bc2be8 · 2018-07-11T14:43:21.000+02:00
diff --git a/lib/server.js b/lib/server.js
@@ -116,9 +116,10 @@ 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) => {
-        config.port = port
+      .then(() => NetUtils.bindAvailablePort(config.port, config.listenAddress))
+      .then((boundServer) => {
+        config.port = boundServer.address().port
+        this._boundServer = boundServer
         this._injector.invoke(this._start, this)
       })
       .catch(this.dieOnError.bind(this))
@@ -156,7 +157,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
@@ -4,29 +4,24 @@ const Promise = require('bluebird')
 const net = require('net')
 
 const NetUtils = {
-  isPortAvailable (port, listenAddress) {
+  bindAvailablePort (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(server)
+        })
+        .listen(port, listenAddress)
     })
-  },
-
-  getAvailablePort (port, listenAddress) {
-    return NetUtils.isPortAvailable(port, listenAddress)
-      .then((available) => available ? port : NetUtils.getAvailablePort(port + 1, listenAddress))
   }
 }
 
diff --git a/package.json b/package.json
@@ -195,6 +195,7 @@
     "Samuel Marks <samuelmarks@gmail.com>",
     "Saugat Acharya <mesaugat@gmail.com>",
     "Schmulik Raskin <schmuli@gmail.com>",
+    "Sergei Startsev <sergei.startsev@gmail.com>",
     "Sergey Kruk <sergey.kruk@gmail.com>",
     "Sergey Simonchik <sergey.simonchik@jetbrains.com>",
     "Seth Rhodes <seth@thinkpixbit.com>",
diff --git 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,13 @@ describe('server', () => {
       }
     }
 
+    mockBoundServer = {
+      on: sinon.spy((event, callback) => callback && callback()),
+      listen: sinon.spy((port, listenAddress, callback) => callback && callback()),
+      close: sinon.spy((callback) => callback && callback()),
+      address: () => { return { port: 9876 } }
+    }
+
     mockWebServer = {
       on (name, handler) {
         if (name === 'error') {
@@ -99,18 +106,24 @@ describe('server', () => {
       close: sinon.spy((callback) => callback && callback())
     }
 
-    sinon.stub(server._injector, 'get')
+    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, 'bindAvailablePort').resolves(mockBoundServer)
+      sinon.stub(mockBoundServer, 'address').returns({ port: 9877 })
+      sinon
+        .stub(server, 'get')
+        .withArgs('config').returns(config)
     })
 
     it('should compile static resources', (done) => {
@@ -123,7 +136,17 @@ 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.bindAvailablePort).to.have.been.calledWith(9876, '127.0.0.1')
+        expect(mockBoundServer.address).to.have.been.called
+        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()
       })
     })
@@ -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)
 
@@ -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,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)
 
diff --git a/test/unit/utils/net-utils.spec.js b/test/unit/utils/net-utils.spec.js
@@ -4,46 +4,33 @@ 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.bindAvailablePort', () => {
+  it('resolves server with bound port when it is available', (done) => {
+    NetUtils.bindAvailablePort(9876, '127.0.0.1').then((boundServer) => {
+      const port = boundServer.address().port
+      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.bindAvailablePort(9876, '127.0.0.1').then((boundServer) => {
+        const port = boundServer.address().port
+        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.bindAvailablePort(9876, incorrectAddress).catch((err) => {
+      expect(err).not.to.be.null
       done()
     })
   })
