Skip to content
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

DRIVERS-1785: clarify ordering of CMAP events when pool is cleared #1690

Merged
merged 5 commits into from
Nov 12, 2024

Conversation

baileympearson
Copy link
Contributor

@baileympearson baileympearson commented Oct 28, 2024

Node POC: mongodb/node-mongodb-native#4296

This PR adds tests ensuring that drivers always clear the pool before checking a connection back in / emitting close events, regardless of if the pool cleared error occurs on establishment during minPoolSize population, connection checkout or during operation execution on a checked out connection.


This PR originally intended to just add a test that demonstrated that, when we encounter a pool clear error on an application connection, we clear the pool before checking the connection back into the pool. This avoids a small race condition between SDAM and connection checkout for subsequent requests in the pool - the subsequent requests might check out the errored connection before the pool is cleared.

As I was looking into this, I did realize we do have this coverage already. But as Matt pointed out on the drivers ticket, the ordering of events seemed to be specified differently in one CMAP minPoolSize test than in other CMAP and SDAM tests. The race condition isn't present for minPoolSize population, but Matt pointed out that if we expect a different ordering of events for minPoolSize population and connection checkout, we can't share connection establishment logic in both code paths. So I also updated the ordering of events for the incorrect CMAP test (pool clear must happen before connectionClosed events).

I also realized some tests only tested that handshakes clear the pool, while others test that authentication clears the pool. I added tests for both errors during the handshake and auth during minPoolSize population and connection checkout, and assert on the expected ordering of events.

Please complete the following before merging:

  • Update changelog.
  • Make sure there are generated JSON files from the YAML test files.
  • Test changes in at least one language driver.
  • Test these changes against all server versions and topologies (including standalone, replica set, sharded
    clusters, and serverless).

@baileympearson baileympearson changed the title DRIVERS-1785: add test to ensure pool is cleared before connection check-in DRIVERS-1785: clarify ordering of CMAP events when pool is cleared Oct 29, 2024
@baileympearson baileympearson marked this pull request as ready for review October 29, 2024 18:27
@baileympearson baileympearson requested review from a team as code owners October 29, 2024 18:27
@baileympearson baileympearson requested review from W-A-James, stIncMale, matthewdale and dariakp and removed request for a team October 29, 2024 18:27
Copy link
Contributor

@matthewdale matthewdale left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@W-A-James W-A-James requested a review from ShaneHarvey October 30, 2024 22:37
@baileympearson baileympearson merged commit 376b98a into mongodb:master Nov 12, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants