race condition when used simultaneously across multiple processes

[pmuellr]

@AkihiroKitada opened an issue on https://github.com/cloudfoundry-community/node-cfenv - issue https://github.com/cloudfoundry-community/node-cfenv/issues/40

As near as I can tell, this is from the ports package, here:

https://github.com/hoodiehq-archive/node-ports/blob/4af2255b26fadea5fe5cc77a68625d0ab7d6cc0f/index.js#L52-L55

It appears that if multiple processes are simultaneously accessing the ports package, presumably writing the same entry out the first time, you could be in a position where the writes are happening first, then a rename runs, then the next rename runs but the file has already been renamed, so it generates an error.

Some thoughts:

• ports should probably survive this situation, but I think notify the user on the write error. It could do this by returning an error object from write_json. It currently always returns undefined, and since it's going to be a rare occurrence, I think extending the return value range this way is fine. And then anyone who cares about the success of the function can check the result

• I think you can still have invalid race conditions that don't throw an error. getPort('a') and getPort('b') both returning the "next" open port, but one of them overwriting the the other so only one gets set in the ports.json file

[pmuellr]

Here's a script you can use to repro:

'use strict'

const APPS = 3
const WORKERS = 100

const cluster = require('cluster')

const ports = require('ports')

cluster.isMaster ? runMaster() : runWorker()

function runMaster () {
  const baseAppName = `app-${Math.round(Math.random() * 1000000)}`
  process.env.BASE_APP_NAME = baseAppName

  let runningWorkers = 0

  for (let i = 0; i < WORKERS; i++) {
    runningWorkers++
    const worker = cluster.fork()

    worker.on('exit', () => {
      runningWorkers--

      if (runningWorkers <= 0) process.exit(0)
    })
  }
}

function runWorker () {
  const appName = `${process.env.BASE_APP_NAME}-${cluster.worker.id % APPS}`

  let port
  try {
    port = ports.getPort(appName)
  } catch (err) {
    console.log(`ports.getPorts(${appName}): ${err.message}`)
    process.exit(0)
  }

  console.log(`ports.getPorts(${appName}): ${port}`)
  process.exit(0)
}

Here's a run of the script catching some of the race conditions:

$ node tests/ports/stress.js | sort | uniq
ports.getPorts(app-611308-0): 6041
ports.getPorts(app-611308-0): 6042
ports.getPorts(app-611308-1): 6040
ports.getPorts(app-611308-2): 6041
ports.getPorts(app-611308-2): ENOENT: no such file or directory, rename '/Users/pmuellr/.ports.json.tmp' -> '/Users/pmuellr/.ports.json'

In this case, we can see that some runs of app-611308-0 returned 6041, some 6042, but should have just returned 6042 I think. Some runs of app-611308-2 also returned 6041 and also hit the rename race condition.