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

feat: Implement random port retry logic #1577

Closed
wants to merge 7 commits into from

Conversation

u9520107
Copy link

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

Port finding itself doesn't seem to have any test written already. If someone can suggest how to structure the test for this scenario, I can try and set something up.

Motivation / Use-Case

Trying to support #1572.
The main change is to delay port finding to just before trying to run server.listen. Just this change alone reduced the time window between determining the available port and actually listening to it, as previously the compiler is initiated between this time frame. This naturally let most instances attempt to get available port in different moments and reducing the risk of trying to use the same port.
Another change is to listen to error event when port finding is used, when 'EADDRINUSE' error is encountered, retry the port finding again for up to 10 times. Not the most elegant solution, but should be sufficient for most prototyping scenarios.
If retried for 10 times and still cannot find available pot to use, the dev-server will error out like before.

Breaking Changes

Unless there are people relying on the fact that attempting to start multiple instances without specifying port could result in errors, this should not have breaking changes.

Additional Info

This is my first contribution here, please comment if there are things that I should add.

@jsf-clabot
Copy link

jsf-clabot commented Nov 22, 2018

CLA assistant check
All committers have signed the CLA.

@u9520107
Copy link
Author

Can't tell what caused the segment error on that particular instance. Ran the tests on my mac as well. Any advice?

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Need tests too

bin/webpack-dev-server.js Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Dec 3, 2018

Codecov Report

Merging #1577 into master will increase coverage by 0.31%.
The diff coverage is 80.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1577      +/-   ##
==========================================
+ Coverage    75.1%   75.42%   +0.31%     
==========================================
  Files          10       10              
  Lines         687      708      +21     
==========================================
+ Hits          516      534      +18     
- Misses        171      174       +3
Impacted Files Coverage Δ
bin/webpack-dev-server.js 58.19% <66.66%> (-0.1%) ⬇️
bin/utils.js 60% <89.47%> (+15.55%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4767a31...6d10da9. Read the comment docs.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Looks very good, thanks!

bin/utils.js Show resolved Hide resolved
Copy link
Contributor

@odinho odinho left a comment

Choose a reason for hiding this comment

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

@u9520107 If you rebase onto newest master, the tests should hopefully not fail.

bin/webpack-dev-server.js Outdated Show resolved Hide resolved
@u9520107
Copy link
Author

force-pushed after rebase to current master.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Good work, thanks! Some notes

bin/webpack-dev-server.js Show resolved Hide resolved
bin/webpack-dev-server.js Outdated Show resolved Hide resolved
@alexander-akait
Copy link
Member

Ping me when PR was ready for review again, thanks!

@u9520107
Copy link
Author

@evilebottnawi Done, just waiting for CI to finish tests.

@alexander-akait
Copy link
Member

Good job, thanks!

@alexander-akait
Copy link
Member

/cc @mistic Can you help with review?

Copy link
Contributor

@mistic mistic left a comment

Choose a reason for hiding this comment

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

As I said in the comment, aside from a little NIT, it LGTM

}

function findPort(server, defaultPort, defaultPortRetry, fn) {
let tryCount = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

@u9520107 Just a simple NIT here, I think the code could be more readable in the following way, but feel free to not apply the changes!

function runPortFinder(tryCount, defaultPort, cb) {
  portfinder.basePort = defaultPort;
  tryCount += 1;
  portfinder.getPort((err, port) => {
    cb(err, port);
  });
}

function findPort(server, defaultPort, defaultPortRetry, fn) {
  let tryCount = 0;

  server.listeningApp.on('error', (err) => {
    if (err && err.code !== 'EADDRINUSE') {
      throw err;
    }

    if (tryCount >= defaultPortRetry) {
      fn(err);
      return;
    }

    runPortFinder(tryCount, defaultPort, fn);
  });

  runPortFinder(tryCount, defaultPort, fn);
}

@alexander-akait
Copy link
Member

/cc @u9520107 can you rebase?

@mistic mistic mentioned this pull request Feb 25, 2019
6 tasks
@alexander-akait
Copy link
Member

Done in #1692

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

Successfully merging this pull request may close these issues.

None yet

6 participants