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(NODE-5106): prevent multiple mongo client connect()s from leaking topology #3596

Merged
merged 11 commits into from
Mar 28, 2023

Conversation

clemclx
Copy link
Contributor

@clemclx clemclx commented Mar 10, 2023

fix(NODE-5106): Add MongoClient connect lock mechanism to avoid topology leak

Description

Related to the ticket opened here : https://jira.mongodb.org/browse/NODE-5106
As mentionned in the issue, not test provided because atm a test cannot be performed according to the beforeHooks awaiting the MongoClient connect.

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

Copy link
Contributor

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

This generally seems like the right thing to do, but I think you'll need to add a test for this. Somebody from the team can probably confirm.

You probably don't need all the complexity of the resource_clean_up.test.ts file, I think you could just ensure that e.g. when you call client.connect() twice in the same tick, you'll stil only get one open event on the MongoClient.

src/mongo_client.ts Outdated Show resolved Hide resolved
@clemclx
Copy link
Contributor Author

clemclx commented Mar 11, 2023

Yes I clarified in the ticket why I did not add a test.
Maybe it is not necessary to add it to the resource_clean_up.test.ts yes, I let someone of the team with a much better knowledge of the code base decide where it belongs.

I remain available ☺️

@baileympearson baileympearson added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Mar 13, 2023
src/mongo_client.ts Show resolved Hide resolved
@dariakp dariakp added the External Submission PR submitted from outside the team label Mar 15, 2023
@baileympearson baileympearson added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Mar 21, 2023
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

Thanks so much @clemclx really appreciate your help with this! I've got some ideas to improve the testing a bit here:

test/integration/mongo_client.test.ts Outdated Show resolved Hide resolved
test/integration/mongo_client.test.ts Outdated Show resolved Hide resolved
test/integration/mongo_client.test.ts Outdated Show resolved Hide resolved
test/integration/mongo_client.test.ts Outdated Show resolved Hide resolved
test/integration/mongo_client.test.ts Outdated Show resolved Hide resolved
src/mongo_client.ts Outdated Show resolved Hide resolved
test/integration/mongo_client.test.ts Outdated Show resolved Hide resolved
test/integration/mongo_client.test.ts Outdated Show resolved Hide resolved
test/integration/mongo_client.test.ts Outdated Show resolved Hide resolved
test/integration/mongo_client.test.ts Outdated Show resolved Hide resolved
clemclx and others added 5 commits March 23, 2023 20:38
Co-authored-by: Neal Beeken <neal.beeken@mongodb.com>
Co-authored-by: Neal Beeken <neal.beeken@mongodb.com>
Co-authored-by: Neal Beeken <neal.beeken@mongodb.com>
Co-authored-by: Neal Beeken <neal.beeken@mongodb.com>
@clemclx
Copy link
Contributor Author

clemclx commented Mar 28, 2023

Hey @nbbeeken,
I updated the PR according to your review, thanks !
Let me know if I can be useful for something else.

nbbeeken
nbbeeken previously approved these changes Mar 28, 2023
@nbbeeken nbbeeken requested a review from baileympearson March 28, 2023 15:40
@baileympearson baileympearson changed the title fix(NODE-5106): Add MongoClient connect lock mechanism to avoid topology leak fix(NODE-5106): prevent multiple mongo client connect()s from leaking topology Mar 28, 2023
@baileympearson baileympearson merged commit eb836bb into mongodb:main Mar 28, 2023
baileympearson added a commit that referenced this pull request Jun 1, 2023
… topology (#3596)

Co-authored-by: Neal Beeken <neal.beeken@mongodb.com>
Co-authored-by: Bailey Pearson <bailey.pearson@mongodb.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
External Submission PR submitted from outside the team Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants