fix: check for available port before start server (fix #1476, fix #3011) #3039

lusarz · 2018-06-11T12:54:58Z

create NetUtils with isPortAvailable and getAvailablePort methods
use NetUtils to search for available port before starting server and build middlewares
remove handler for EADDRINUSE error - with NetUtils.getAvailablePort we'll know that port is available for sure, so this error would no raise

johnjbarton · 2018-06-11T15:12:24Z

lib/server.js

+        })
+      })
+      .catch((error) => {
+        this.log.error('Front-end script compile failed with error: ' + error)


I think using log4js for fatal errors is insufficient: the message gives no stack trace pointing to the problem.
How about adding console.error('Front-end script compile failed with error: \n', error) (note comma).

johnjbarton · 2018-06-11T15:17:44Z

lib/server.js

-        throw e
-      }
-    })
+    webServer.on('error', (e) => { throw e })


As a reader I have to ponder what will happen here. Maybe we can have a dieOnError(err) function that calls log4js, console.error and the process exit dance, then use it here and the compile fail.

I definitely agree, but I'm not sure this pull request is right place to do it. It may require more investigation.

sergei-startsev · 2018-06-12T16:40:58Z

lib/utils/net-utils.js

+      const server = net.createServer()
+
+      server.unref()
+      server.on('error', () => resolve(false))


I suppose we have to check EADDRINUSE, EACCES codes, otherwise reject the promise and terminate karma:

.on('error', function(e) { if (e.code != 'EADDRINUSE' && e.code!='EACCES'){ return reject(e); } resolve(false)

Do you mean any specific situation when such check would be relevant ? I'm not aware what else kind of errors we may expect here - I assumed it is ok to check next port no matter what kind of error occurs here.

Probably it would be good to limit number of checks. What do you think ?

As an example you could specify invalid listenAddress in karma.config: listenAddress: '123'. It should fail with EADDRNOTAVAIL error.

So, I suppose it would be safer to limit checks using known errors related to port usage.

@sergei-startsev
I've updated pull request with your suggestion. What do you think about current solution ?

@lusarz thanks for the updates, I've left a few more comments that might be useful.

sergei-startsev · 2018-06-12T16:42:00Z

lib/utils/net-utils.js

+      server.unref()
+      server.on('error', () => resolve(false))
+
+      server.listen({ port }, () => {


We have to know whether a process is listening on a port, not just whether the port was available. These aren't exactly the same thing. I believe you also have to specify the host to prevent EACCES:

server.listen(port, '0.0.0.0', ()=>{...});

Thanks, good point with host - I've updated pull request.

I think isPortAvailable function actually checks whether other process is listening on a port. isPortAvailable try to run some basic server, if everything is ok it close it and resolves with true.

It seems to be enough, but I may miss something.

johnjbarton · 2018-06-12T23:18:06Z

lib/utils/net-utils.js

+  getAvailablePort (port, listenAddress) {
+    return NetUtils.isPortAvailable(port, listenAddress).then((available) => {
+      if (available) return Promise.resolve(port)
+      else return NetUtils.getAvailablePort(port + 1)


If config.port starts out of bounds for ports (> 65535), then we will loop until the test times out somehow. This will appear as a hang. I think we should at least log on the else path. Maybe we should limit the quest to say 100 calls.

@johnjbarton there will be RangeError for ports start out of bounds:

RangeError: "port" argument must be >= 0 and < 65536

So I think the approach suggested above will also work properly here:

if (e.code != 'EADDRINUSE' && e.code!='EACCES'){ return reject(e); }

ok great. (I'm unclear on the state of the code here so I'll let you do the merge when ready)

sergei-startsev · 2018-06-13T22:30:08Z

lib/server.js

@@ -47,6 +48,12 @@ function createSocketIoServer (webServer, executor, config) {
  return server
 }

+function dieOnError (error) {
+  console.error(error)


should it be this.log.error instead?

@johnjbarton suggested here:
#3039 (comment)

to use console.error to log fatal errors. I think it's ok.

hmm... I suppose this.log.error was initially used here https://github.com/karma-runner/karma/pull/3039/files/193ccdd1147b58ee2117322a105155aea064827e#diff-c945a46d13b34fcaff544d966cffcabaL157

If we would like to replace this.log.error by console.error for fatal errors, it should be consistent across the module, see https://github.com/karma-runner/karma/pull/3039/files/193ccdd1147b58ee2117322a105155aea064827e#diff-c945a46d13b34fcaff544d966cffcabaR160

Otherwise I would leave the existing behavior in that PR.

I've used dieOnError for that case now. Now use of console.error for fatal errors is consistent in this module.

sergei-startsev · 2018-06-13T22:31:09Z

lib/server.js

-          this.log.error('Front-end script compile failed with error: ' + error)
+        }
+        if (this.loadErrors.length > 0) {
+          this.log.error('Found %d load error%s', this.loadErrors.length, this.loadErrors.length === 1 ? '' : 's')


could dieOnError be used here?

sergei-startsev · 2018-06-13T22:34:54Z

lib/server.js

+      BundleUtils.bundleResourceIfNotExist('client/main.js', 'static/karma.js'),
+      BundleUtils.bundleResourceIfNotExist('context/main.js', 'static/context.js')
+    ])
+      .then(() => NetUtils.getAvailablePort(this.get('config.port'), this.get('config.listenAddress')))


does it reasonable to get config and reuse it below instead of getting it each time?

const config = this.get('config') ... .then(() => NetUtils.getAvailablePort(config.port, config.listenAddress)) ... config.port = port

sergei-startsev · 2018-06-13T22:39:24Z

lib/utils/net-utils.js

+
+      server.unref()
+      server.on('error', (err) => {
+        server.close()


I suppose it's redundant

Are you sure ? I see in nodejs documentation (a bit above https://nodejs.org/api/net.html#net_server_listen_handle_backlog_callback) that author invoke server.close() inside server.on('error', (e) => {}) - to be honest I don't know why and whether it's necessary.

https://nodejs.org/api/net.html#net_event_error
well, it seems reasonable to leave it

sergei-startsev · 2018-06-13T22:47:59Z

lib/utils/net-utils.js

+  },
+
+  getAvailablePort (port, listenAddress, maxAttempts = 10) {
+    if (!maxAttempts) {


I believe you can omit this check since you've added err.code === 'EADDRINUSE' || err.code === 'EACCES' check. 10 attempts aren't enough if you have hundreds test suites and you would like to parallelize them on multi-core machine

Alright, RangeError should save us here. I'm gonna remove this maxAttempts check.

sergei-startsev · 2018-06-13T22:50:58Z

test/unit/utils/net-utils.spec.js

+
+const NetUtils = require('../../../lib/utils/net-utils')
+const connect = require('connect')
+const http = require('http')


might use net server, it's not so heavy

Good point, fixed

sergei-startsev

I've left a couple of minor comments, otherwise looks good to me.

sergei-startsev · 2018-06-14T22:20:44Z

test/unit/utils/net-utils.spec.js

+
+const NetUtils = require('../../../lib/utils/net-utils')
+const connect = require('connect')
+const http = require('net')


const net = require('net')

sergei-startsev · 2018-06-14T22:23:03Z

lib/utils/net-utils.js

+      .then((available) => {
+        if (available) {
+          return port
+        } else {


else is redundant

…3011)

johnjbarton suggested changes Jun 11, 2018

View reviewed changes

johnjbarton approved these changes Jun 12, 2018

View reviewed changes

sergei-startsev reviewed Jun 12, 2018

View reviewed changes

johnjbarton reviewed Jun 12, 2018

View reviewed changes

sergei-startsev suggested changes Jun 13, 2018

View reviewed changes

sergei-startsev suggested changes Jun 14, 2018

View reviewed changes

fix(server): check available port before start server (fix #1476, fix #…

8137c61

…3011)

lusarz merged commit a19b8d4 into karma-runner:master Jun 15, 2018

lusarz deleted the find-available-port-before-start-karma branch October 11, 2018 06:37

loftyduck mentioned this pull request Jan 9, 2023

[Snyk] Fix for 1 vulnerabilities loftyduck/karma#10

Open

enterstudio mentioned this pull request Jan 10, 2023

[Snyk] Fix for 1 vulnerabilities enterstudio/karma#11

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: check for available port before start server (fix #1476, fix #3011) #3039

fix: check for available port before start server (fix #1476, fix #3011) #3039

lusarz commented Jun 11, 2018 •

edited

Loading

johnjbarton Jun 11, 2018

lusarz Jun 11, 2018

johnjbarton Jun 11, 2018

lusarz Jun 11, 2018

sergei-startsev Jun 12, 2018

lusarz Jun 12, 2018 •

edited

Loading

sergei-startsev Jun 12, 2018

sergei-startsev Jun 12, 2018

lusarz Jun 13, 2018 •

edited

Loading

sergei-startsev Jun 13, 2018

sergei-startsev Jun 12, 2018

lusarz Jun 12, 2018 •

edited

Loading

johnjbarton Jun 12, 2018

sergei-startsev Jun 13, 2018

johnjbarton Jun 13, 2018

sergei-startsev Jun 13, 2018

lusarz Jun 14, 2018

sergei-startsev Jun 14, 2018

lusarz Jun 14, 2018

sergei-startsev Jun 13, 2018

lusarz Jun 14, 2018

sergei-startsev Jun 13, 2018

lusarz Jun 14, 2018

sergei-startsev Jun 13, 2018

lusarz Jun 14, 2018

sergei-startsev Jun 14, 2018

sergei-startsev Jun 13, 2018

lusarz Jun 14, 2018

lusarz Jun 14, 2018

sergei-startsev Jun 13, 2018

lusarz Jun 14, 2018

sergei-startsev left a comment

sergei-startsev Jun 14, 2018

sergei-startsev Jun 14, 2018

fix: check for available port before start server (fix #1476, fix #3011) #3039

fix: check for available port before start server (fix #1476, fix #3011) #3039

Conversation

lusarz commented Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lusarz Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lusarz Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lusarz Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergei-startsev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lusarz commented Jun 11, 2018 •

edited

Loading

lusarz Jun 12, 2018 •

edited

Loading

lusarz Jun 13, 2018 •

edited

Loading

lusarz Jun 12, 2018 •

edited

Loading