refactor(NODE-5915): refactor topology close logic to be synchronous #4021

W-A-James · 2024-03-05T21:06:30Z

Description

What is changing?

Refactored the following functions/methods to be synchronous:
- Connection.destroy
- ConnectionPool.close
- Server.destroy
- Topology.close
Removed DestroyOptions as the force flag is unused in all the places it would have been passed in
Reordered connection check in and call to Connection.onError in ConnectionPool.interruptInUseConnections due to failure on this test
Updated relevant tests to accommodate newly synchronous functions

Is there new documentation needed for these changes?

No

What is the motivation for this change?

NODE-5915

Release Highlight

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

…hronous

… github.com:mongodb/node-mongodb-native into NODE-5915/refactor-topology-close-to-be-synchronous

…hronous

W-A-James · 2024-03-07T21:41:10Z

src/cmap/connection_pool.ts

        connection.onError(new PoolClearedOnNetworkError(this));
+        this.checkIn(connection);


@baileympearson this was done to fix a test failure where we were getting an uncaught error when running this spec test

Yeah, this is right. Good catch.

I think the reason this worked in the past was because

checkIn calls destroyConnection(), which calls connection.destroy() in a process.nextTick()

then `connection.onError() runs and shuts down the connection

then connection.destroy() is a no-op because the connection has already been destroyed

You made destroyConnection synchronous, so we need to run onError first to ensure the connection is closed with the correct error. Then we run checkIn, which resets the pool state and calls connection.destroy(), which is a no-op.

aditi-khare-mongoDB

left a few small comments, otherwise LGTM, nice work :)

aditi-khare-mongoDB · 2024-03-08T18:43:17Z

src/cmap/connection_pool.ts

Let's file a follow-up ticket in the V7 epic to remove closeOptions since its no longer used anywhere in the code base after these changes.

Good callout here. I can file the follow-up and the first subtask of that follow-up can be to mark it as deprecated.

Looking a little closer, it seems the only places that interface was used are in Topology.close and ConnectionPool.close which are both internal classes, so users shouldn't be interacting with this interface at all. Seems like it wouldn't be a breaking change to just remove it in this PR along with DestroyOptions.

Unfortunately, even though it's only used in internal classes / types, it's public and exported so technically it's a part of our API. In situations like this in the past, we've considered it breaking to remove and done it in a major release

aditi-khare-mongoDB · 2024-03-08T18:45:12Z

src/cmap/connection_pool.ts

@@ -28,7 +28,6 @@ import { CancellationToken, TypedEventEmitter } from '../mongo_types';
 import type { Server } from '../sdam/server';
 import {
  type Callback,
-  eachAsync,


The eachAsync utils function is now also used nowhere in the codebase, do we want to remove it? It uses callbacks, and IIUC we no longer want to add any new callback code to the driver.

Good point. I agree that removing it is likely the way we want to go here. @nbbeeken @baileympearson thoughts?

aditi-khare-mongoDB

code changes LGTM! I can't seem to find the follow-up ticket, can you link the follow-up link on this ticket.

W-A-James · 2024-03-08T21:15:03Z

Follow-up ticket: NODE-6007

baileympearson · 2024-03-08T21:19:25Z

src/mongo_client.ts

+    topology.close();
+
+    const { encrypter } = this[kOptions];
+    if (encrypter) {
+      await encrypter.close(this, force);
+    }


can close() throw? If so, both the new and old implementations potentially would leave the encrypter open if topology.close() throws

Thanks for the investigation into whether these can throw or not.

doesn't have to be in this PR but do you think it might be worth handling errors anyways, since we could introduce errors into close() in the future?

baileympearson · 2024-03-08T21:21:42Z

src/sdam/server.ts

+    this.pool.close();
+    stateTransition(this, STATE_CLOSED);
+    this.emit('closed');


same question as with client.close() - can pool.close() throw?

baileympearson · 2024-03-08T21:22:34Z

src/sdam/topology.ts

@@ -469,7 +469,8 @@ export class Topology extends TypedEventEmitter<TopologyEvents> {
      selectServerOptions,
      (err, server) => {
        if (err) {
-          return this.close({ force: false }, () => exitWithError(err));
+          this.close();


same question about errors

method relevant direct dependencies throws?

Connection.destroy - No

ConnectionPool.close Connection.destroy No

Server.destroy stateTransition*, ConnectionPool.close No; stateTransition* can technically throw, but Server.destroy is written such that it will never try to make an illegal state transition.

destroyServer Server.destroy No

Topology.close destroyServer, stateTransition** No; stateTransition** can technically throw, but like in Server.destroy, Topology.close is written such that it never tries to make an illegal state transition

Note:

stateTransition* refers to the local function using the static state machine defined for the Server class created by the makeStateMachine helper.

stateTransition** refers to the local function using the static state machine defined for the Topology class created by the makeStateMachine helper.

baileympearson · 2024-03-08T21:49:36Z

src/cmap/connection_pool.ts

        connection.onError(new PoolClearedOnNetworkError(this));
+        this.checkIn(connection);


Yeah, this is right. Good catch.

I think the reason this worked in the past was because

checkIn calls destroyConnection(), which calls connection.destroy() in a process.nextTick()

then `connection.onError() runs and shuts down the connection

then connection.destroy() is a no-op because the connection has already been destroyed

You made destroyConnection synchronous, so we need to run onError first to ensure the connection is closed with the correct error. Then we run checkIn, which resets the pool state and calls connection.destroy(), which is a no-op.

…hronous

W-A-James added 15 commits March 5, 2024 15:57

refactor(NODE-5915): refactor topology close logic to be synchronous

c105a58

Merge branch 'main' into NODE-5915/refactor-topology-close-to-be-sync…

15d1b7a

…hronous

remove extraneous async logic

7d97dfa

fix tests

fb7d617

fix hanging

f69ab81

eslint and build fixes

d1f3817

Merge branch 'main' into NODE-5915/refactor-topology-close-to-be-sync…

b5edf02

…hronous

fix test runner

16b4bd6

remove force flag

4122b0a

Merge branch 'NODE-5915/refactor-topology-close-to-be-synchronous' of…

48fa90a

… github.com:mongodb/node-mongodb-native into NODE-5915/refactor-topology-close-to-be-synchronous

fix test

e0a68ae

remove import

6aa97e0

Merge branch 'main' into NODE-5915/refactor-topology-close-to-be-sync…

3b40d9f

…hronous

fix????

9f1d474

remove DestroyOptions

64da97f

W-A-James marked this pull request as ready for review March 7, 2024 21:52

Merge branch 'main' into NODE-5915/refactor-topology-close-to-be-sync…

b923d25

…hronous

W-A-James commented Mar 7, 2024

View reviewed changes

aditi-khare-mongoDB self-requested a review March 8, 2024 16:46

aditi-khare-mongoDB added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Mar 8, 2024

aditi-khare-mongoDB requested changes Mar 8, 2024

View reviewed changes

remove eachAsync

b9ee91c

W-A-James requested a review from aditi-khare-mongoDB March 8, 2024 21:04

W-A-James force-pushed the NODE-5915/refactor-topology-close-to-be-synchronous branch from c533616 to b9ee91c Compare March 8, 2024 21:08

aditi-khare-mongoDB reviewed Mar 8, 2024

View reviewed changes

aditi-khare-mongoDB self-requested a review March 8, 2024 21:11

aditi-khare-mongoDB 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 8, 2024

aditi-khare-mongoDB approved these changes Mar 8, 2024

View reviewed changes

nbbeeken requested a review from baileympearson March 8, 2024 21:13

baileympearson requested changes Mar 8, 2024

View reviewed changes

W-A-James requested a review from baileympearson March 8, 2024 23:17

nbbeeken approved these changes Mar 11, 2024

View reviewed changes

baileympearson approved these changes Mar 11, 2024

View reviewed changes

Merge branch 'main' into NODE-5915/refactor-topology-close-to-be-sync…

aebb2da

…hronous

nbbeeken merged commit 36fa752 into main Mar 11, 2024
21 of 25 checks passed

nbbeeken deleted the NODE-5915/refactor-topology-close-to-be-synchronous branch March 11, 2024 17:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(NODE-5915): refactor topology close logic to be synchronous #4021

refactor(NODE-5915): refactor topology close logic to be synchronous #4021

W-A-James commented Mar 5, 2024 •

edited

W-A-James Mar 7, 2024

baileympearson Mar 8, 2024

aditi-khare-mongoDB left a comment

aditi-khare-mongoDB Mar 8, 2024 •

edited

W-A-James Mar 8, 2024

W-A-James Mar 8, 2024

baileympearson Mar 8, 2024

aditi-khare-mongoDB Mar 8, 2024 •

edited

W-A-James Mar 8, 2024

nbbeeken Mar 8, 2024

aditi-khare-mongoDB left a comment

W-A-James commented Mar 8, 2024

baileympearson Mar 8, 2024

baileympearson Mar 11, 2024

baileympearson Mar 8, 2024

baileympearson Mar 8, 2024

W-A-James Mar 8, 2024

baileympearson Mar 8, 2024

		connection.onError(new PoolClearedOnNetworkError(this));
		this.checkIn(connection);

method	relevant direct dependencies	throws?
`Connection.destroy`	-	No
`ConnectionPool.close`	`Connection.destroy`	No
`Server.destroy`	`stateTransition`*, `ConnectionPool.close`	No; `stateTransition`* can technically throw, but `Server.destroy` is written such that it will never try to make an illegal state transition.
`destroyServer`	`Server.destroy`	No
`Topology.close`	`destroyServer`, `stateTransition`**	No; `stateTransition`** can technically throw, but like in `Server.destroy`, `Topology.close` is written such that it never tries to make an illegal state transition

refactor(NODE-5915): refactor topology close logic to be synchronous #4021

refactor(NODE-5915): refactor topology close logic to be synchronous #4021

Conversation

W-A-James commented Mar 5, 2024 • edited

Description

What is changing?

Is there new documentation needed for these changes?

What is the motivation for this change?

Release Highlight

Double check the following

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aditi-khare-mongoDB left a comment

Choose a reason for hiding this comment

aditi-khare-mongoDB Mar 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aditi-khare-mongoDB Mar 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aditi-khare-mongoDB left a comment

Choose a reason for hiding this comment

W-A-James commented Mar 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

W-A-James commented Mar 5, 2024 •

edited

aditi-khare-mongoDB Mar 8, 2024 •

edited

aditi-khare-mongoDB Mar 8, 2024 •

edited