Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix get-port #52

Closed
wants to merge 4 commits into from
Closed

fix get-port #52

wants to merge 4 commits into from

Conversation

jeetiss
Copy link

@jeetiss jeetiss commented Feb 11, 2021

get-port doesn't check port availability for all hosts in some environments, so it return port that already in use

i fix that with next steps:

  • get all existed hosts by os.networkInterfaces()
  • check port with this hosts
  • some hosts can't be used for user land servers and throws EADDRNOTAVAIL on mac or EINVAL on linux, so i remove them for all next tests

@sindresorhus
Copy link
Owner

What does it fix? Do you intend to finish this PR?

@jeetiss
Copy link
Author

jeetiss commented Mar 9, 2021

Do you intend to finish this PR?

yes

What does it fix?

Looks like get-port return ports that are not available on macos(?).
on GHA all tests are green, but on my laptop one of them red:


  1 test failed

  preferred ports is bound up with different hosts

  /Users/jeetiss/Projects/get-port/test.js:171

   170:                               
   171:   t.is(port, desiredPorts[2]);
   172: });                           

  Difference:

  - 10991
  + 10992

@jeetiss jeetiss marked this pull request as ready for review March 9, 2021 11:06
index.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Looks like get-port return ports that are not available on macos(?).
on GHA all tests are green, but on my laptop one of them red:

Please fill out the pull request description field with what it fixes and how it fixes the problem.

it's dangerous because some iterations are skipped
await getAvailablePort({port: options.port, host}); // eslint-disable-line no-await-in-loop
} catch (error) {
if (['EADDRNOTAVAIL', 'EINVAL'].includes(error.code)) {
hosts.splice(hosts.indexOf(host), 1);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mutates the passed-in array, which can cause problems later on. Copy the array at the top of this function before using it.

const port = await getPort({port: desiredPorts});

t.is(port, desiredPorts[2]);
});
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have liked to see more comprehensive testing than a single assertion.

@sindresorhus
Copy link
Owner

sindresorhus commented Mar 24, 2021

get-port doesn't check port availability for all hosts in some environments, so it return port that already in use

Are you sure that's a get-port bug and not a Node.js bug? Just want to ensure we don't fix this in the wrong place.

@sindresorhus
Copy link
Owner

Bump :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants