Apply the WithTransaction context deadline to nested AbortTransaction calls

XMLWordPrintableJSON

    • Type: Improvement
    • Resolution: Unresolved
    • Priority: Unknown
    • None
    • Affects Version/s: None
    • Component/s: Transactions
    • None
    • None
    • Go Drivers
    • None
    • None
    • None
    • None
    • None
    • None

      `WithTransaction` may execute many operations and, on failure, will call `AbortTransaction` from the commit/abort loop. As of now, the driver intentionally ignores the context deadline/cancellation when calling `AbortTransaction` so that transaction cleanup can run even if the user operation has already timed out.

      we currently skip the 'withTransaction' test in this test suite since we have not implemented timeoutMS for withTransaction (use to repro, but this is not a bug):

      go test -run TestUnifiedSpec/client-side-operations-timeout/tests/sessions-inherit-timeoutMS.json ./internal/integration/unified -v
      

      The underlying motivation for applying the per-op timeout is that we should prioritize how Go Developers expect Go timeouts to work. However, if we do end up applying this timeout, we want to investigate if we risk inadequate communication between the server and client of what resources have been cleaned-up**/used for an operation, and if that could lead to errors such as max session handles reached.

      **Note: If client-side abort fails, the server has its own transaction timeout: either set by the user using transactionLifetimeLimitSeconds or a default 60 seconds. The transaction will eventually be cleaned up server-side. So if we did pass a context with a deadline to AbortTransaction and it timed out before the operation could be called, then resources will still be cleaned up.

      Definition of done: what must be done to consider the task complete?

      this is an investigative ticket, we need to see if timeoutMS to session.WithTransaction can be feasibly applied:

      ctx, cancel := csot.WithTimeout(ctx, s.client.timeout)
      defer cancel()
      
      • Decide whether nested `AbortTransaction` calls in `WithTransaction` should:
        • continue to ignore context deadline/cancellation (current behavior), or
        • use the same context deadline that was passed to WithTransaction
      • Document the decision with an in-code comment in the `WithTransaction` commit/abort loop explaining the rationale for future developers.
      • Answer the historical question: why did we decide to ignore deadline/cancellation for `AbortTransaction` in the first place (e.g. to guarantee transaction cleanup even after user timeouts)?
      • If we keep the current behavior, ensure the tests that assume a specific `timeoutMS` behavior for `WithTransaction` are updated or unskipped to match the documented decision.

      (note) IF we do end up applying the timeout, the following tests should be removed from the skip-list:

      "TestUnifiedSpec/client-side-operations-timeout/tests/sessions-inherit-timeoutMS.json/timeoutMS_applied_to_withTransaction"
      

            Assignee:
            Ayesha Islam
            Reporter:
            Matt Dale
            None
            Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

              Created:
              Updated: