-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat: Implement random port retry logic #1577
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
Conversation
Can't tell what caused the segment error on that particular instance. Ran the tests on my mac as well. Any advice? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need tests too
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this 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!
There was a problem hiding this 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.
168b785
to
33c8c13
Compare
force-pushed after rebase to current master. |
There was a problem hiding this 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
Ping me when PR was ready for review again, thanks! |
@evilebottnawi Done, just waiting for CI to finish tests. |
Good job, thanks! |
/cc @mistic Can you help with review? |
There was a problem hiding this 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; |
There was a problem hiding this comment.
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);
}
/cc @u9520107 can you rebase? |
Done in #1692 |
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.