refactor(NODE-4633): executeOperation to use maybeCallback #3421

nbbeeken · 2022-09-21T21:51:20Z

Description

NODE-4633

What is changing?

package-lock.json and package.json
- Pulls in the latest node types to fix lint, should disappear when chore(NODE-4661): bump @types/node version to fix ts next #3422 merges
src/bulk/common.ts
- contains the refactor discussed in refactor(NODE-4631): change_stream, gridfs to use maybeCallback #3406 removes the two usages of maybeCallback for one unified one, made possible by the executeOperation PR.
src/collection.ts and src/operations/rename.ts and src/operations/stats.ts and src/operations/delete.ts
- The promisify type was incompatible with these operation classes because they don't pass their types into the super class generic. Delete was able to be fixed the others lead to numerous TS issues, this all relates to NODE-3286.
src/operations/command.ts
- The override was redundant, could lead to mistakes down the road where the return type generic doesn't make it to the super class.
src/operations/execute_operation.ts
- The whole reason we're here 😸 Leaving executeOperation nearly empty to just call a new TBD named async function was intentional to avoid causing renames across tons of files, but we can do a follow up if we want to do that.
- ~~Perhaps we don't need to run serverSelection twice?~~ (session support check)
  - No we do, we need to use a difference selector than the user / operation may require.
- Added a helper to get TS to think topology always exists (it will since we call connect)
- retryOperation is a helper function now, this isn't a big change it's just not defined inline, so we have to pass it the variables it needs instead of it binding to the current scope.
  - retyability is undergoing a change with CSOT so I think more refactor + feature work will improve this further
src/operations/operation.ts
- Creates the promisified wrapper for .execute()
src/sdam/server.ts
- noResponse was causing endSessions to ALWAYS throw, which we squash, but it prevented executeOperation from retrying. Fixed.
src/sdam/topology.ts
- Makes a promisified wrapper for serverSelection, I would happy to make this a proper async function in a follow up. It's not super complex but will have a significant impact on tests so best to do it on its own.
test/integration/crud/bulk.test.ts
- ⚠️ Forgot to await an insertMany here async/await makes this always fail now. fixed!
test/integration/crud/maxTimeMS.test.ts
- ⚠️ Forgot to await an insertMany here async/await makes this always fail now. fixed!
- Added printing stack traces to assertions.
test/integration/server-selection/operation_count.test.ts
- the operationCount used to be reached synchronously, we need to add a sleep here to allow the code to reach the increment
test/integration/sessions/sessions.spec.prose.test.ts and test/integration/sessions/sessions.test.ts
- Our minimum session usage for all the parallel operations we run in these tests is now 2. With async await we will never put the first created session back before the next operation attempts to pull one out of the pool. Unlike multithreaded drivers that may have some level of nondeterminism, connection checkIn -> release is deterministically after the next operation reaches the checkOut -> acquire step. As we further refactor to async await I don't expect this to go any higher than 2 but it is possible it could decrease again to 1 if the order of async steps allows us to return the session before the next operation tries to acquire one.

What is the motivation for this change?

async await is easier to reason about, error wise, ordering wise, and forces consistent execution.

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

src/operations/execute_operation.ts

baileympearson · 2022-09-26T20:53:42Z

src/operations/execute_operation.ts

+}
+
+const assertTopology = async (client: MongoClient) => {
+  const { topology } = client;


related - function names should describe what the function does and assertTopology isn't descriptive of what this function does. This function:

connects the client

asserts the topology is connected

returns the topology

At the least, can we use a better name? connectClientAndAssertTopology? That's a mouthful, so I'd probably do the following (just organizational preference)

rename the function connectIfNotConnected or something like this and make it return void so that the function

move the if (topology == null) check to a regular if check outside the function

I renamed and improved the control flow here, keeping the assertion in here though seems like an acceptable approach because it's really unreachable to have a nullish topology after connect.

I think we should move away from the naming convention maybe<something> because it's not particularly descriptive and after v5, we won't have need for maybeCallback anymore.

maybe even just autoConnect would be better but we could also just move this code back inline - it's only used in once place and isn't terribly complicated so I see no problem with having it inline

baileympearson · 2022-09-26T20:54:54Z

src/operations/execute_operation.ts

+    }
+    client.s.options[Symbol.for('@@mdb.skipPingOnConnect')] = true;
+    await client.connect();
+    delete client.s.options[Symbol.for('@@mdb.skipPingOnConnect')];


I think we need to put this in a finally so that we remove this regardless of whether or not connect fails (that's also the old behavior).

src/operations/execute_operation.ts

src/operations/rename.ts

src/operations/stats.ts

src/operations/execute_operation.ts

baileympearson · 2022-09-28T13:51:47Z

src/operations/execute_operation.ts

+}
+
+const assertTopology = async (client: MongoClient) => {
+  const { topology } = client;


I think we should move away from the naming convention maybe<something> because it's not particularly descriptive and after v5, we won't have need for maybeCallback anymore.

maybe even just autoConnect would be better but we could also just move this code back inline - it's only used in once place and isn't terribly complicated so I see no problem with having it inline

test/integration/sessions/sessions.spec.prose.test.ts

)

nbbeeken force-pushed the NODE-4633/refactor-executeOperation branch 9 times, most recently from f2c0ad2 to e3d3a1b Compare September 26, 2022 19:04

nbbeeken marked this pull request as ready for review September 26, 2022 19:52

baileympearson reviewed Sep 26, 2022

View reviewed changes

src/operations/execute_operation.ts Outdated Show resolved Hide resolved

baileympearson requested changes Sep 27, 2022

View reviewed changes

nbbeeken requested a review from baileympearson September 27, 2022 17:31

baileympearson requested changes Sep 28, 2022

View reviewed changes

nbbeeken added 8 commits September 28, 2022 12:54

refactor(NODE-4633): executeOperation to use maybeCallback

ec3e349

test: skip serverless test

517d2ed

fix: serverless, add return-await rule

32d69e6

fix: some comments

b8a6c2b

test: add comment

9e241fa

fixup finally, simple if stmt

fa45443

break up null session, and non retryable, flip if cond

6f05d02

inline auto connect, simple bool

0148f5e

nbbeeken force-pushed the NODE-4633/refactor-executeOperation branch from 218eb8f to 0148f5e Compare September 28, 2022 16:54

nbbeeken requested a review from baileympearson September 28, 2022 16:56

baileympearson previously approved these changes Sep 28, 2022

View reviewed changes

baileympearson added the Team Review Needs review from team label Sep 28, 2022

fix: condition (hasWriteAspect && willRetryWrite)

62019f6

nbbeeken dismissed baileympearson’s stale review via 62019f6 September 28, 2022 17:32

dariakp reviewed Sep 28, 2022

View reviewed changes

test/integration/sessions/sessions.spec.prose.test.ts Show resolved Hide resolved

dariakp approved these changes Sep 28, 2022

View reviewed changes

baileympearson approved these changes Sep 28, 2022

View reviewed changes

baileympearson merged commit 492456b into main Sep 28, 2022

baileympearson deleted the NODE-4633/refactor-executeOperation branch September 28, 2022 18:25

ZLY201 pushed a commit to ZLY201/node-mongodb-native that referenced this pull request Nov 5, 2022

refactor(NODE-4633): executeOperation to use maybeCallback (mongodb#3421

24aae44

)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(NODE-4633): executeOperation to use maybeCallback #3421

refactor(NODE-4633): executeOperation to use maybeCallback #3421

Uh oh!

nbbeeken commented Sep 21, 2022 •

edited

Loading

Uh oh!

Uh oh!

baileympearson Sep 26, 2022

Uh oh!

nbbeeken Sep 27, 2022

Uh oh!

baileympearson Sep 28, 2022

Uh oh!

nbbeeken Sep 28, 2022

Uh oh!

baileympearson Sep 26, 2022

Uh oh!

nbbeeken Sep 27, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

baileympearson Sep 28, 2022

Uh oh!

Uh oh!

Uh oh!

refactor(NODE-4633): executeOperation to use maybeCallback #3421

refactor(NODE-4633): executeOperation to use maybeCallback #3421

Uh oh!

Conversation

nbbeeken commented Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

What is changing?

What is the motivation for this change?

Double check the following

Uh oh!

Uh oh!

baileympearson Sep 26, 2022

Choose a reason for hiding this comment

Uh oh!

nbbeeken Sep 27, 2022

Choose a reason for hiding this comment

Uh oh!

baileympearson Sep 28, 2022

Choose a reason for hiding this comment

Uh oh!

nbbeeken Sep 28, 2022

Choose a reason for hiding this comment

Uh oh!

baileympearson Sep 26, 2022

Choose a reason for hiding this comment

Uh oh!

nbbeeken Sep 27, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

baileympearson Sep 28, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nbbeeken commented Sep 21, 2022 •

edited

Loading