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

feat(NODE-6313): add CSOT support to sessions and transactions #4199

Merged
merged 28 commits into from
Sep 9, 2024

Conversation

nbbeeken
Copy link
Contributor

@nbbeeken nbbeeken commented Aug 16, 2024

Description

What is changing?

  • CSOT support for session APIs

    • abort/commit Transaction
    • endSession
    • withTransaction
      • Despite the diff I only:
        • indented the logic into a try/finally block
        • the timeoutContext is always cleared when withTxn is done
        • the old timeout mechanism is short-circuited away if we have a timeoutContext defined
    • API docs updated.
  • Some external fixes to timeout handling:

    • Do not synchronously throw from getters that return promises (timeouts) so .then chaining works as expected
    • Move TimeoutError conversion out of onData into readMany so that the stack trace is preserved
    • update mongodb-legacy version to pull in options passing fix
    • Moved duration into timeout error for debugging, it should always be equal to timeoutMS but it is useful when it is not. If we don't like this, we can revert it now or keep it on the CSOT feature, and file a follow up to remove

Testing:

  • Pulled in changes to spec test files, all that changed was some bumps to timeouts to make them less flakey
  • Implemented the prose tests and following suit with the above spec, the timeouts are a bit longer than suggested as I observed inserts timing out before TXN operations could be reached (the APIs we were aiming to timeout/test)
  • A few unified runner changes to support the tests.

TBD: I have to find out what is going on with 4.4 replica sets. I will open the PR now to start review. The same tests pass on all other variants, and the failure is that abortTransaction is not failing despite having a failpoint set. It could be something I am doing wrong, but I am suspicious that the blockConnection failpoint seems to hit our timeout logic in all other servers. 🤔

Is there new documentation needed for these changes?

Yes

What is the motivation for this change?

Transactions can be conveniently timed out. And sessions respect a single timeout setting from either the client or defaultTimeoutMS to deadline the completion of a Txn

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

@nbbeeken nbbeeken marked this pull request as ready for review August 21, 2024 21:12
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.

Comments.

Copy link
Contributor Author

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

Half comments addressed

  • need to follow up on the prose tests ms changes, we have to bump them or our inserts fail before the txn operation under test is reached.
  • abortTransaction's error suppression will be followed up on, will file a ticket about using disposeStack

test/tools/unified-spec-runner/operations.ts Outdated Show resolved Hide resolved
src/timeout.ts Show resolved Hide resolved
src/sessions.ts Show resolved Hide resolved
src/sessions.ts Outdated Show resolved Hide resolved
src/operations/execute_operation.ts Show resolved Hide resolved
src/sessions.ts Show resolved Hide resolved
src/operations/execute_operation.ts Outdated Show resolved Hide resolved
src/sessions.ts Show resolved Hide resolved
src/operations/execute_operation.ts Show resolved Hide resolved
src/sessions.ts Outdated Show resolved Hide resolved
src/sessions.ts Outdated Show resolved Hide resolved
@nbbeeken nbbeeken added the Team Review Needs review from team label Aug 28, 2024
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.

Sorry for the delay in this review. Just a few small comments, otherwise looks good

src/sessions.ts Show resolved Hide resolved
src/sessions.ts Show resolved Hide resolved
src/sessions.ts Show resolved Hide resolved
@@ -647,96 +709,119 @@ export class ClientSession
*/
async withTransaction<T = any>(
fn: WithTransactionCallback<T>,
options?: TransactionOptions
options?: TransactionOptions & {
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we're missing a doc requirement:

Drivers MUST also document that overriding timeoutMS for operations executed using the explicit session inside the provided callback will result in a client-side error, as defined in Validation and Overrides.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Overriding timeoutMS for operations executed using the explicit session inside the provided callback will result in a client-side error.

I have this ^ I figure putting this on the api doc for timeoutMS in this function makes it the most visible to where it is relevant (like vscode hover as they add a timeoutMS option here)

@baileympearson baileympearson merged commit 1c48970 into NODE-6090 Sep 9, 2024
24 of 27 checks passed
@baileympearson baileympearson deleted the NODE-5687-csot-sessions branch September 9, 2024 15:11
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.

3 participants