http-party / node-portfinder

A simple tool to find an open port or domain socket on the current machine
https://github.com/http-party/node-portfinder
Other
887 stars 95 forks source link

the version 1.0.22 do not search from the basePort #84

Closed fubai closed 5 years ago

fubai commented 5 years ago

the version 1.0.22 do not search from the basePort

BarthesSimpson commented 5 years ago

It does, but instead of starting at basePort p and checking p, p+1, p+2... stopPort in order, it now checks a random sequence of ports in the range basePort...stopPort. This change was to fix an issue where calling getPort more than once in quick succession could lead to the same port being returned multiple times if the client had not finished binding by the time the second call ran its check (https://github.com/http-party/node-portfinder/issues/79).

bcfurtado commented 5 years ago

Hi, this seems to be is a breaking change then. Previously, the expected behavior was that the basePort would be used as the initial default value. Packages such as vue-cli and [webpack-dev] were relying on the old behavior.

$ npm list | grep portfinder
│ ├─┬ portfinder@1.0.22
│ │ ├── portfinder@1.0.22 deduped

$ node --experimental-repl-await
> const portfinder = require('portfinder')
undefined
> portfinder.basePort
8000
> let port = await portfinder.getPortPromise()
undefined
> port
10211
$ npm list | grep portfinder
│ ├── portfinder@1.0.21 deduped
│ │ ├── portfinder@1.0.21 deduped
├─┬ portfinder@1.0.21

$ node --experimental-repl-await
> const portfinder = require('portfinder')
undefined
> portfinder.basePort
8000
> let port = await portfinder.getPortPromise()
undefined
> port
8000
jaredjj3 commented 5 years ago

It does, but instead of starting at basePort p and checking p, p+1, p+2... stopPort in order, it now checks a random sequence of ports in the range basePort...stopPort. This change was to fix an issue where calling getPort more than once in quick succession could lead to the same port being returned multiple times if the client had not finished binding by the time the second call ran its check (#79).

The change made does not solve the problem, since multiple calls of getPort can still return the same port. This "port collision" event is more probable as the range basePort...stopPort decreases to 1.

I recommend reverting this change or making it optional. The caller will then be responsible for port reservation.

AndreasCag commented 5 years ago

I recently configured webpack and was really frustrated with that behaviour.

As for me, I would like to have my app on a certain port most of the time, and if it is busy, then choose next port in a sequential order. It is easy to remember and convenient behaviour.

ThibaultVlacich commented 5 years ago

It does, but instead of starting at basePort p and checking p, p+1, p+2... stopPort in order, it now checks a random sequence of ports in the range basePort...stopPort. This change was to fix an issue where calling getPort more than once in quick succession could lead to the same port being returned multiple times if the client had not finished binding by the time the second call ran its check (#79).

This absolutely makes no sense.

1/ There's no bug in the first place. If an application "call[s] getPort more than once in quick succession" there's a problem with the logic, as you should wait for a call to have returned a port, assign it, and then make an other call. 2/ Even if, it doesn't "fix" the issue, it just makes the probably of it happening very thin.

And seriously, releasing a breaking change like that as a "patch"...

BarthesSimpson commented 5 years ago

1/ There's no bug in the first place. If an application "call[s] getPort more than once in quick succession" there's a problem with the logic, as you should wait for a call to have returned a port, assign it, and then make an other call.

It's not about waiting for the port to return - it's that whatever is binding to the returned port (e.g. a server) can take some time to bind even after the port is returned.

2/ Even if, it doesn't "fix" the issue, it just makes the probably of it happening very thin.

That's correct. It means any application that is binding and releasing a large number of ports dynamically can rely on a simple error handler in the downstream logic (i.e. if the attempt to bind encounters an EADDRINUSE, retry) which will be invoked roughly once every 32,000 calls rather than having to block on each binding attempt until successful binding is verified.

It seems like a lot of consumers are relying on the sequential scan though, and there is a PR out that reverts to making that the default behavior: https://github.com/http-party/node-portfinder/pull/86

ctstewart commented 5 years ago

I use Vue Cli, and this completely broke devServer.port in vue.config.js. I ended up reverting to 1.0.21 of portfinder for now.

lltx commented 5 years ago

Very bad

Qaick commented 5 years ago

Also https://github.com/vuejs/vue-cli/issues/4452

pimterry commented 5 years ago

This has created bugs in my application too - I'm specifically depending on finding the first available port in the range, not a random port.

eriktrom commented 5 years ago

I will roll back immediately

eriktrom commented 5 years ago

Reverted v1.0.22 - v1.0.23 is the same as v1.0.21 now