feat(NODE-6882): close outstanding connections

[nbbeeken]

Description

What is changing?

• connections that are in-use are interrupted by client.close to gracefully shutdown node

Is there new documentation needed for these changes?

Yes

What is the motivation for this change?

Graceful event loop exit

Release Highlight

MongoClient close shuts outstanding in-use connections

The MongoClient.close() method now shuts connections that are in-use allowing the event loop to close if the only remaining resource was the MongoClient.

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

[baileympearson]

Can we follow a similar pattern to interruptInUseConnection, where we error the connection and then check it in, relying on the checkIn logic to emit the appropriate error? (I actually think we're missing a checkIn event here, with the current implementation).

What should happen is:

• connection is destroyed
• connection is checked in
• checkIn event emitted
• connection is errored, so we destroy the connection (using destroyConnection()) (this already handles emitting a connectionClosedEvent with the correct configuration when the pool is closed.)

edit: I see you must do this manually, because you're killing connections before the pool has been closed. This question still stands, because I'd expect server.close() to be the method responsible for cleaning up each pool.

[nbbeeken]

I think my reply to the other comment thread clarifies the reason for this, lmk if not.

I added a test to demonstrate we're emitting check-ins as expected, the handling for check in already happens where the operation is being awaited in server.command()

[baileympearson]

I still think we should have exactly one mechanism for interrupting in-use connections. I think that it probably doesn't make sense to share a method, because the other interruptInUse method has particular conditions about what it closes (i.e, pool generation requirements) but we should use the same pattern in both cases.

Here's the current implementation of the old interruptInUse:

for (const connection of this.checkedOut) {
  if (connection.generation <= minGeneration) {
    connection.onError(new PoolClearedOnNetworkError(this));
    this.checkIn(connection);
  }
}

iirc, the implementation was important for two reasons:

1. according to the spec tests, check in events must be emitted before connection closed events (this is handled by checkIn())
2. ordering of these events relative to other events matters, so we needed to process them synchronously per-connection (i.e., call checkIn() manually instead of letting the connection destroy itself, and relying on the error handling in server.ts to check the connection back in)

Even if we don't consider 2. to be important for your current PR, I think we should ensure 1. is satisfied. It currently isn't, because we manually emit the closed event before destroying the connection. So at a minimum, I'd suggest removing that logic. I'd also suggest using the same logic as we use in interruptInUse already, for consistency.

Thoughts?

[baileympearson]

(and related to my last comment): can we assert on the ordering of checkin + close events in the test you added?

[nbbeeken]

Updated to assert the ordering of events and simplified the helper on the pool to make that the case.

[nbbeeken]

Why should we call checkIn twice? we should be able to remove the check in call from interruptInUse

[baileympearson]

I answered that above, although we can re-evaluate now I suppose because the connection layer has changed. But:

iirc, the implementation was important for two reasons:

1. according to the spec tests, check in events must be emitted before connection closed events (this is handled by checkIn())
2. ordering of these events relative to other events matters, so we needed to process them synchronously per-connection (i.e., call checkIn() manually instead of letting the connection destroy itself, and relying on the error handling in server.ts to check the connection back in)

checking in immediately ensures that we get a checkIn + closed event synchronously

[nbbeeken]

checkin and close events are both emitted from the checkIn() function so they're always emitted one after the other synchronously, which part is async? Is there a test I can write to assert this behavior?

[baileympearson]

Try making the change and seeing if anything fails? Maybe nothing will fail. My memory is failing me and I don't remember exactly why this was so important / which tests failed.

Sorry for not explaining thoroughly though. The issue was particularly with the ordering of events relative to other pool + sdam events, not the immediacy of a checkIn + close event for the same connection (although that is also important, that won't change because checkIn is synchronous). The checkin + close events will always be next to each other, you're right. but there's nothing that guarantees that, if we remove the call to checkIn from interruptInUse, that the checkIn+close will happen immediately with no other events before it

[nbbeeken]

Removed checkIn from interruptInUse and nothing is failing

[baileympearson]

I'm confused by the ordering of events here. Why are we explicitly closing in-use connections before we do other cleanup logic? I'd expect that this would just be handled automatically by topology.close(). Using topology.close() also means that we don't need to add new methods to the topology + server classes, just for this use case.

[baileympearson]

Could this also lead to connection establishments during close? Hypothetically, if we kill all connections in a pool, killCursors or endSessions will require establishing new connections for these commands.

[nbbeeken]

Why are we explicitly closing in-use connections before we do other cleanup logic?

To avoid a deadlock, since we're about to run operations (killCursors, abortTransaction, endSessions) we can deadlock if the pool is at maximum use (ex. 100 checked out connections). First closing in-use connections means we have a pool that is ready for use so we can proceed to clean up operations.

I'd expect that this would just be handled automatically by topology.close()

Maybe this was answered by the previous paragraph but to be clear we still need an open and operating topology/servers/pool/connections to be able to perform best-effort clean-up.

Could this also lead to connection establishments during close?

Yes, but this is not new, a MongoClient can already have empty pools when we call MongoClient close running clean up operations will create new connections. Even thought we're closing in-use operations that doesn't mean that we will need to make a new connection, the pool can still have idle connections that won't be closed by this.

[baileympearson]

Alternate approach: since cleanup is best-effort, could we instead just attempt the commands we need to send with a low waitQueueTimeoutMS set to set an upper bound on how long we wait in case of a deadlock?

[nbbeeken]

The same proposal could be made for the killCursors run inside cursor.close() (not in here, client.close(), but normally by the user).

But I think the reason that we do "risk" the full waitQueueTimeoutMS and serverSelectionTimeoutMS is because I don't think we can predict good timeout values that will fit some common denominator given the driver is general purpose and deployed in many ways, so we don't have a common case to approach (ex. the timing for mongosh is vastly different from an edge server app).

Closing in-use connection separately is specifically so our connection count isn't at maxPoolSize when we reach the next steps.

[baileympearson]

Can we add a test demonstrating that closing connections pre-killCursors prevents the deadlock you mentioned? Probably worth capturing in a test.

[nbbeeken]

Added a test, and I checked if I comment out closeCheckedOutConnections then it fails

[baileympearson]

I still think we should have exactly one mechanism for interrupting in-use connections. I think that it probably doesn't make sense to share a method, because the other interruptInUse method has particular conditions about what it closes (i.e, pool generation requirements) but we should use the same pattern in both cases.

Here's the current implementation of the old interruptInUse:

for (const connection of this.checkedOut) {
  if (connection.generation <= minGeneration) {
    connection.onError(new PoolClearedOnNetworkError(this));
    this.checkIn(connection);
  }
}

iirc, the implementation was important for two reasons:

1. according to the spec tests, check in events must be emitted before connection closed events (this is handled by checkIn())
2. ordering of these events relative to other events matters, so we needed to process them synchronously per-connection (i.e., call checkIn() manually instead of letting the connection destroy itself, and relying on the error handling in server.ts to check the connection back in)

Even if we don't consider 2. to be important for your current PR, I think we should ensure 1. is satisfied. It currently isn't, because we manually emit the closed event before destroying the connection. So at a minimum, I'd suggest removing that logic. I'd also suggest using the same logic as we use in interruptInUse already, for consistency.

Thoughts?

[baileympearson]

Alternate approach: since cleanup is best-effort, could we instead just attempt the commands we need to send with a low waitQueueTimeoutMS set to set an upper bound on how long we wait in case of a deadlock?

[baileympearson]

(and related to my last comment): can we assert on the ordering of checkin + close events in the test you added?

[baileympearson]

Suggested change

	constructor(message = 'Operation interrupted because client was closed') {
	constructor() {

suggest: don't bother with a message, we only construct this in one place with the same error every time.

[nbbeeken]

No message at all? that seems unconventional

[baileympearson]

No, just hard code the message on the error. There's no reason to provide it in the constructor if we only use it in one place and always expect it to be the same.

[nbbeeken]

Oh gotcha

[baileympearson]

suggest: can we just deep assert on the events, like the UTR does?

expect(
  allClientEvents.map(event => event.name)
).to.deep.equal(
  [checkout, checkout, checkout, checkIn, close, checkIn, close, checkIn, close]
)

Or is there something that asserting on the actual connection ids captures here that my proposed assertion doesn't?

[nbbeeken]

There's no assertions on the ids they're just filtering for the insert related checkins/outs/closes. There's more events because close is running endSessions and there's a connection created for the initial ping. So there's just a handful of things that aren't relevant to the test and would need revising if there were unrelated details changed about the other operations run