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(getport): Randomize first port using crypto #844

Merged
merged 2 commits into from
Jan 13, 2024

Conversation

noseworthy
Copy link
Contributor

The current implementation of getPort() relies on using Date.now()to get the first port to try to launch mongo on, even when the EXP_NET0LISTEN flag is set. This causes a couple issues:

  • If the Date module is mocked, it can result in getFreePort() returning the same first port every time.
  • If multiple mongos are being started at once in parallel, it's possible for the same first port to be picked leading to port conflicts

In order to better randomize the initial port selection so that port conflicts can be avoided, instead of using Date.now() for an initial seed, use crypto.randomInt() which should provide more randomness and hopefully avoid some of these race conditions.

Related Issues

The current implementation of `getPort()` relies on using `Date.now()`
to get the first port to try to launch mongo on, even when the
`EXP_NET0LISTEN` flag is set. This causes a couple issues:
- If the `Date` module is mocked, it can result in `getFreePort()`
  returning the same first port every time.
- If multiple mongos are being started at once in parallel, it's
  possible for the same first port to be picked leading to port
  conflicts
In order to better randomize the initial port selection so that port
conflicts can be avoided, instead of using `Date.now()` for an initial
seed, use `crypto.randomInt()` which should provide more randomness and
hopefully avoid some of these race conditions.

Fixes typegoose#827
Copy link

codecov bot commented Jan 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f5c7e27) 91.22% compared to head (b84c82a) 91.24%.
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #844      +/-   ##
==========================================
+ Coverage   91.22%   91.24%   +0.01%     
==========================================
  Files          15       15              
  Lines        1984     1988       +4     
  Branches      499      501       +2     
==========================================
+ Hits         1810     1814       +4     
  Misses        165      165              
  Partials        9        9              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

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

Thanks, didnt know about the crypto function (though i knew about Math.random, but for some reason i cant remember didnt use it)

@hasezoey hasezoey merged commit 4132dbf into typegoose:master Jan 13, 2024
9 checks passed
Copy link

@github-actions github-actions bot added the released Pull Request released | Issue is fixed label Jan 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement released Pull Request released | Issue is fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants