Skip to content

refactor(NODE-4632): async await in MongoClient, ClientSession, and AbstractCursor #3428

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

Merged
merged 14 commits into from
Oct 3, 2022

Conversation

nbbeeken
Copy link
Contributor

@nbbeeken nbbeeken commented Sep 28, 2022

Description

NODE-4632

What is changing?

  • src/change_stream.ts and src/collection.ts and src/cursor/find_cursor.ts and src/db.ts and src/mongo_types.ts and src/operations/drop.ts
    • spelling mistakes
  • src/connection_string.ts and test/unit/connection_string.test.ts and test/unit/mongo_client.test.js
    • refactor resolveSRVRecord to be async, wrap dns APIs in promisify
    • validateLoadBalancedOptions now throws since the error does not need to be passed to a callback
  • src/cursor/abstract_cursor.ts
    • Implement the async iterator using native syntax when a custom promise is not set. I think this is important to do now ensuring is maintains the expected behavior so when we drop custom promise support we don't miss something in the other API removals/changes that would be part of the major.
    • Utilize the async iterator for toArray and forEach
    • promifisify next - we may want to make this really async in a follow up.
    • ditto cleanupCursor
  • src/encrypter.ts
    • async connectInternalClient, this method is internal, does not need to support maybeCallback
  • src/gridfs/download.ts and test/integration/gridfs/gridfs_stream.test.js
    • remove nextTick usage, the callback for close was being called sync, now that we always do things async it changed the order of events. removing nextTick brings us back to that behavior
  • src/gridfs/upload.ts
    • no longer relevant comment
  • src/mongo_client.ts
    • maybeCallback connect - moved the floating function logic into the class, make TS happier and simplifies repition.
      • removed no mongos proxies found condition, this is unreachable, we removed this case in 3.x
    • maybeCallback close - this was mostly using promises already. topology and encrypter close is still wrapped though.
  • src/operations/connect.ts
    • removed, see above
  • src/sessions.ts
    • endSession, commit/abortTransaction async-ified.
    • promisified endTransaction, this one is a bit involved to convert since the finally vs handle the error is carefully part of correct transaction logic. should be done separately.
  • src/utils.ts
    • Deleted maybePromise 🎉 🥳 🍾 🎂
    • changed the name of a variable so that there is no reference to maybePromise, definitelyPromise from here on out
  • test/integration/collation/collations.test.js
    • this test was failing for a bit, fixed the capital
  • test/integration/crud/aggregation.test.ts
    • added a beforeEachClient, and fixed the test the proved we were throwing synchronously ⚠️ This is no longer possible!
  • test/integration/crud/explain.test.js
    • Explain demonstrates an exhaust cursor in one next call, so I had to debug a bug here, test got refactored in the process.
  • test/integration/crud/misc_cursors.test.js
    • toArray and forEach do not throw on exhausted cursors, but .next and async iteration will, test needed debugging got refactored.
  • test/integration/node-specific/cursor_stream.test.js
    • this test was hanging, the end event never came, I refactored but made one change to introduce a call to resume which is what I think was blocking the event. Before our sync call to callbacks would get us the event to emit even if the stream is paused, so this was just incorrect stream usage IIUC.
  • test/integration/server-selection/readpreference.test.js
    • These tests were using sinon to check exact args passes to server selection, the args are slightly different but same logic is being checked.

What is the motivation for this change?

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>
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@nbbeeken nbbeeken changed the base branch from main to NODE-4633/refactor-executeOperation September 28, 2022 17:59
Base automatically changed from NODE-4633/refactor-executeOperation to main September 28, 2022 18:25
@nbbeeken nbbeeken force-pushed the NODE-4632/refactor-client-cursor-session branch 4 times, most recently from 97bbe38 to b1cac7a Compare September 28, 2022 20:17
@nbbeeken nbbeeken force-pushed the NODE-4632/refactor-client-cursor-session branch from b1cac7a to f562a80 Compare September 29, 2022 17:01
@nbbeeken nbbeeken marked this pull request as ready for review September 29, 2022 18:41
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Sep 29, 2022
Copy link
Contributor

@baileympearson baileympearson left a comment

Choose a reason for hiding this comment

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

Looks really good overall, just some questions and minor comments

@@ -526,20 +518,20 @@ function validateLoadBalancedOptions(
hosts: HostAddress[] | string[],
mongoOptions: MongoOptions,
isSrv: boolean
): MongoParseError | undefined {
): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we annotate this function as throwing / add a comment? The error information used to be codified in the return type, but now it's lost.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

});
});
return maybeCallback(async () => {
// If a connection already been established, we can terminate early
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// If a connection already been established, we can terminate early

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@baileympearson baileympearson merged commit 82b4a23 into main Oct 3, 2022
@baileympearson baileympearson deleted the NODE-4632/refactor-client-cursor-session branch October 3, 2022 17:52
ZLY201 pushed a commit to ZLY201/node-mongodb-native that referenced this pull request Nov 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Primary Review In Review with primary reviewer, not yet ready for team's eyes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants