fix(NODE-7411): iterate all servers on close#4967
Draft
johnmtll wants to merge 15 commits into
Draft
Conversation
The method had a premature 'return' inside the for loop, which caused it to only close connections on the first server and exit immediately. This fix removes the return statement so all servers have their checked-out connections closed when MongoClient.close() is called. This bug would affect multi-server topologies (replica sets, sharded clusters) where only the first server's connections would be properly closed.
…deployments - Remove 'single' topology restriction from metadata to support replicaset/sharded - Use readPreference: 'secondaryPreferred' to exercise connections to secondaries - Switch from insert to find operations to validate reads against secondaries
johnmtll
force-pushed
the
NODE-7411/iterate-all-servers-on-close
branch
from
39e064b to
1b7b0c5
Compare
johnmtll
force-pushed
the
NODE-7411/iterate-all-servers-on-close
branch
from
7c08cc1 to
7c109ec
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a shutdown bug in SDAM topology cleanup where MongoClient.close() only closed checked-out connections for the first server in a multi-server topology, and extends integration test coverage to validate resource clearing and CMAP event ordering during client close.
Changes:
- Fix
Topology.closeCheckedOutConnections()to iterate and close checked-out connections for all servers in the topology. - Add/adjust integration tests to verify client resource clearing (cursors/sessions) across connect/close orderings and to validate CMAP event sequencing when closing across multiple pools.
- Add an Evergreen orchestration wrapper script to echo configuration and set PATH for binaries.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/sdam/topology.ts |
Removes premature return so all servers’ checked-out connections are closed on client shutdown |
test/integration/node-specific/client_close.test.ts |
Adds resource-clearing tests and a multi-pool closeCheckedOutConnections CMAP ordering test |
test/integration/connection-monitoring-and-pooling/connection_pool.test.ts |
Adjusts CMAP test to use find failpoint/operations and broadens metadata requirements |
.evergreen/run-orchestration.sh |
Adds wrapper script for drivers-tools orchestration with environment echoing/PATH setup |
auth ops add event noise to event collectors in tests, so now we select only the last 3 events in a connection-affine event collection when conducting assertions
johnmtll
force-pushed
the
NODE-7411/iterate-all-servers-on-close
branch
from
a4e24bf to
1ccfe1d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Summary of Changes
Supplants:
Additional Coverage:
Adds additional coverage for resource clearing to the mongo client tests, ensuring that cursors and sessions are properly wiped after closure, regardless of connection state. Also closes a missing test gap which ensures duplicate/misordered calls of connect()/close() dont cause fatal behaviour (increasing coverage slightly).
Adjusts client closure SDAM tests to ensure event ordering is maintained when a client closure spans 2 connection pools.
Notes for Reviewers
This is a technical glue test fixture duplication (SDAM validation) across connection_pool.test.ts and client_close.test.ts:
Connpool tests validate SDAM requirements at the connpool level, whereas the client-close tests ensure that those requirements persist when a coordinator attempts to close all connections across mutliple connpools. The domain of this 'coordination testing' is explicitly owned by the client close tests.
What is the motivation for this change?
Release Highlight
Release notes highlight
Double check the following
npm run check:lint)type(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript