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

fix(NODE-4262): make startSession work without a connection #3286

Merged
merged 9 commits into from
Jun 15, 2022

Conversation

nbbeeken
Copy link
Contributor

@nbbeeken nbbeeken commented Jun 3, 2022

Description

What is changing?

  • Many test fix ups were needed for this change
  • MongoClient is now the responsible for ClientSessions instead of the topology which only exist after connecting
    • The MongoClient took on the logic for session clean up that was in the topology.
  • Since sessions are connectionless, cursors can obtain a session upon creation. This removes some logic where cursors had to perform server selection to check for session support. This can now be done just at operation time.

What is the motivation for this change?

Now that connect is automatic and since server sessions are created lazily we can allow the API to make sessions before the client has been connected.

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

@nbbeeken nbbeeken force-pushed the NODE-4262/session-start-fix branch 6 times, most recently from 08d6b88 to b32a9d2 Compare June 4, 2022 20:35
@nbbeeken nbbeeken marked this pull request as ready for review June 4, 2022 20:36
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jun 6, 2022
Copy link
Contributor

@baileympearson baileympearson left a comment

Choose a reason for hiding this comment

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

I think I commented all the places I found - but there are a number of tests that are refactored seemingly at random while other tests in the file are not, and a number of tests that are deleted.

Non-test changes look good to me - just some small questions and suggestions 😄

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

test: async function () {
const session = client.startSession({ causalConsistency: true });
collection.insertOne({ darmok: 'jalad' }, { session });
Copy link
Contributor

Choose a reason for hiding this comment

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

unrelated to your work but we should await these functions - especially since they're examples!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not so sure, we should look closer at this test, currently it will always pass

Copy link
Contributor

Choose a reason for hiding this comment

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

This is in the examples folder, so I assumed it's an example that the docs team pulls in?

test/integration/node-specific/db.test.js Show resolved Hide resolved
test/tools/cluster_setup.sh Outdated Show resolved Hide resolved
test/integration/node-specific/db.test.js Show resolved Hide resolved
test/unit/cursor/abstract_cursor.test.ts Show resolved Hide resolved
test/unit/collection.test.ts Show resolved Hide resolved
src/mongo_client.ts Show resolved Hide resolved
@nbbeeken nbbeeken force-pushed the NODE-4262/session-start-fix branch from d820581 to 9f011bc Compare June 14, 2022 18:08
@nbbeeken
Copy link
Contributor Author

Test changes summary:

  • M test/integration/causal-consistency/causal_consistency.prose.test.js
  • skipLeakTests is no longer part of the leak checker, explicit sessions should always be cleaned up, clients should always be closed. Removed usage
  • M test/integration/change-streams/change_stream.test.ts
    • Removed skipLeakTests usage
    • destorying the stream at the should support piping of Change Streams cleans up the change stream, could probably use end but that's asynchronous, would require done/promisifying
    • made afterEach async/await better stack trace when I was fixing a bug
    • made beforeEach async/await that sets up mockServer, better TS, better stack trace
  • M test/integration/connection-monitoring-and-pooling/connection.test.js
    • Simplified an afterEach that cleans up clients. Introduced behavior where if close throws it won't clean up both. (see comment)
    • async/await test connect bad auth test while debugging it
  • M test/integration/crud/misc_cursors.test.js
    • remove skipLeakTests usage
    • async/await while debugging and Test assertion changed to check activeSessions saved on MongoClient.s
      • should return implicit session to pool when client-side cursor exhausts results on initial query
      • should return implicit session to pool when client-side cursor exhausts results after a getMore
  • M test/integration/gridfs/gridfs_stream.test.js
    • Removed skipLeakChecks usage (overly strict topology requirements and redundant comments removed too)
  • M test/integration/load-balancers/load_balancers.spec.test.js
    • Removed LB's bespoke timeout config. This was helpful during testing when we were implementing LB but it overrides the timeout that you provide on the CLI, which I sometimes need to set to 0 when debugging
  • M test/integration/node-specific/db.test.js
    • Deleted test that doesn't offer more coverage than what we have in the connect/close behavior tests.
  • M test/integration/node-specific/examples/causal_consistency.test.js
    • Remove skipLeakTests
  • M test/integration/node-specific/mongo_client.test.ts
    • Add sends endSessions with noResponse set test, ensure we don't wait for MongoDB to reply to this command
  • M test/integration/node-specific/operation_example.test.js
    • Deleted Should correctly connect with default replicasetNoOption test because it tested against the Topology directly, and asserted nothing. By name it sounds like its intention was to cover an SDAM state which is covered in spec tests
    • Drive by assertion improvements in Should correctly rewind and restart cursor while debugging
  • D test/integration/node-specific/topology.test.js
    • Reverting this deletion, although I doubt the merit of testing this internal behavior without more coverage that explains what the states assert about the topology. (not public API, not Spec-ed)
  • M test/integration/sessions/sessions.test.ts
    • Remove skipLeakTests
    • should send endSessions for multiple sessions changed assertion to use the activeSessions on MongoClient
    • session support detection - new suite of tests stubs topology to support/not support sessions, proves that we will omit the session if the topology does not support it
  • M test/integration/shared.js
    • TS fix
  • D test/integration/shared.test.js
    • Removed, all the tests do not clean up properly and the helpers they test are being removed in the follow up "remove connect from tests" PR
  • M test/integration/transactions/transactions.test.ts
    • Use client in place of topology
  • M test/tools/cluster_setup.sh
    • oops!
  • M test/tools/common.js
    • Unused helper can be deleted
  • M test/tools/runner/hooks/configuration.js
    • Add process.pid to print out helps to quickly identify test run to send it signals (did you know if you send SIGUSR1 to node js it opens the debugger? so cool!)
  • M test/tools/unified-spec-runner/runner.ts
    • Remove skipLeakTests
  • M test/unit/collection.test.ts
    • Use async/await in hook, better TS better stack while debugging
  • A test/unit/cursor/abstract_cursor.test.ts
    • Consolidated cursor behavior tests related to sessions into an abstract_cursor test
  • D test/unit/cursor/aggregation_cursor.test.js
    • Tests rewritten in abstract_cursor
  • D test/unit/cursor/find_cursor.test.js
    • Tests rewritten in abstract_cursor
  • M test/unit/sessions.test.js
    • Utilize a client where ever topology was used

Copy link
Contributor

@baileympearson baileympearson left a comment

Choose a reason for hiding this comment

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

Assuming CI passes (it did last time), LGTM

@nbbeeken nbbeeken added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Jun 14, 2022
@baileympearson baileympearson merged commit 89ad7c3 into main Jun 15, 2022
@baileympearson baileympearson deleted the NODE-4262/session-start-fix branch June 15, 2022 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants