CSHARP-3552: CSOT: Transactions #1745

sanych-sun · 2025-08-01T18:21:05Z

No description provided.

rstam

Proposed changes prototyped here:

https://github.com/rstam/mongo-csharp-driver/tree/chsarp3552-0807

rstam · 2025-08-07T18:51:24Z

src/MongoDB.Driver/Core/Bindings/ICoreSessionExtensions.cs

+namespace MongoDB.Driver.Core.Bindings
+{
+    // TODO: CSOT: Make it public when CSOT will be ready for GA
+    internal static class ICoreSessionExtensions


I recommend creating a new ICoreSessionInternal interface to hold the new methods we hope to eventually add to ICoreSession.

These extension methods are useful to make it "look" (to us) like the new methods are already part of ICoreSession.

These extension methods should cast session to ICoreSessionInternal instead of to an open ended set of concrete implementations.

All classes that implement ICoreSession (directly or indirectly) should also now implement ICoreSessionInternal.

Done. Thank you!

rstam · 2025-08-07T18:52:08Z

src/MongoDB.Driver/Core/Bindings/ICoreSessionExtensions.cs

+        {
+            if (options == null || session.Options.DefaultTransactionOptions?.Timeout == options.Timeout)
+            {
+                session.AbortTransaction(cancellationToken);


Forwarding the new method to the old method doesn't seem necessary but it also doesn't do any real harm other than adding more lines of code that probably aren't needed.

I've decided to have this forwarding, just in case there are any external implementation of ICoreSession, this forwarding might reduce risk of breaking client's code.

rstam · 2025-08-07T18:54:26Z

src/MongoDB.Driver/Core/Bindings/WrappingCoreSession.cs

@@ -182,17 +182,25 @@ public ICoreSession Wrapped

        // public methods
        /// <inheritdoc />
-        public virtual void AbortTransaction(CancellationToken cancellationToken = default(CancellationToken))
+        public virtual void AbortTransaction(CancellationToken cancellationToken = default)
+            => AbortTransaction(null, cancellationToken);


I would have left this method unchanged and forwarded to:

_wrapped.AbortTransaction(cancellationToken);

Let the wrapped class decide whether to forward to the new method or not.

rstam · 2025-08-07T18:55:00Z

src/MongoDB.Driver/Core/Bindings/WrappingCoreSession.cs

        }

        /// <inheritdoc />
-        public virtual Task AbortTransactionAsync(CancellationToken cancellationToken = default(CancellationToken))
+        public virtual Task AbortTransactionAsync(CancellationToken cancellationToken = default)
+            => AbortTransactionAsync(null, cancellationToken);


rstam · 2025-08-07T18:55:15Z

src/MongoDB.Driver/Core/Bindings/WrappingCoreSession.cs

@@ -223,17 +231,25 @@ public long AdvanceTransactionNumber()
        }

        /// <inheritdoc />
-        public virtual void CommitTransaction(CancellationToken cancellationToken = default(CancellationToken))
+        public virtual void CommitTransaction(CancellationToken cancellationToken = default)
+            => CommitTransaction(null, cancellationToken);


rstam · 2025-08-07T18:57:29Z

src/MongoDB.Driver/Core/Misc/SystemWatch.cs

+
+namespace MongoDB.Driver.Core.Misc
+{
+    internal sealed class SystemWatch : IWatch


I think this class should be called SystemStopwatch.

rstam · 2025-08-07T18:59:37Z

src/MongoDB.Driver/IClientSessionExtensions.cs

 {
-    public static ReadPreference GetEffectiveReadPreference(this IClientSession session, ReadPreference defaultReadPreference)
+    // TODO: CSOT: Make it public when CSOT will be ready for GA
+    internal static class IClientSessionExtensions


I recommend adding an interface called IClientSessionInternal to hold all the internal methods that we plan to eventually move to IClientSession.

These extension methods should be casting session to IClientSessionInternal instead of to an unknown number of concrete implementations. (Yes there is only one implementation now, but that's not the point).

Done. Thank you!

rstam · 2025-08-07T19:00:01Z

src/MongoDB.Driver/OperationContext.cs

@@ -85,7 +85,11 @@ public CancellationToken CombinedCancellationToken
                return _combinedCancellationTokenSource.Token;
            }
        }
-        private Stopwatch Stopwatch { get; }
+        private IWatch Watch { get; }


I think this property should be called Stopwatch.

rstam · 2025-08-07T19:00:30Z

tests/MongoDB.Driver.Tests/Core/Misc/FrozenClock.cs

+        public IWatch StartWatch()
+        {
+            var startTime = _utcNow;
+            var mock = new Mock<IWatch>();


Consider renaming mock to mockStopwatch.

rstam · 2025-08-07T19:01:10Z

tests/MongoDB.Driver.Tests/ClientSessionHandleTests.cs

@@ -734,8 +699,11 @@ private Mock<IClock> CreateClockMock(DateTime now, bool[] isRetryAttemptsWithTim
            }

            var mockClock = new Mock<IClock>();
+            var watchMock = new Mock<IWatch>();


Consider renaming watchMock to mockStopwatch.

Same in other places like this.

adelinowona

Partial review. Will provide next review in a bit

adelinowona · 2025-08-07T18:52:24Z

src/MongoDB.Driver/Core/Bindings/CoreSession.cs

+        public Task CommitTransactionAsync(CancellationToken cancellationToken = default)
+            => CommitTransactionAsync(null, cancellationToken);
+
+        // TODO: CSOT: Make it public when CSOT will be ready for GA
+        internal async Task CommitTransactionAsync(CommitTransactionOptions options, CancellationToken cancellationToken = default(CancellationToken))


Inconsistent default parameter syntax for cancellationToken for both methods. Standardize on either default or default(CancellationToken)

adelinowona · 2025-08-07T19:13:59Z

src/MongoDB.Driver/Core/Bindings/WrappingCoreSession.cs

+            => CommitTransactionAsync(null, cancellationToken);
+
+        // TODO: CSOT: Make it public when CSOT will be ready for GA
+        internal virtual Task CommitTransactionAsync(CommitTransactionOptions options, CancellationToken cancellationToken = default(CancellationToken))


should be cancellationToken = default for consistency

adelinowona · 2025-08-07T19:30:39Z

src/MongoDB.Driver/Core/Operations/EndTransactionOperation.cs

Why doesn't abortTransaction also override its createCommand method?

Simply because base class is doing everything what it needs.

adelinowona · 2025-08-07T19:40:16Z

src/MongoDB.Driver/Core/TransactionOptions.cs

do we also need to a With() overload that takes the timeout parameter?

We might, but not sure if we want to keep this With pattern. We might want to add it for consistency. @BorisDog WDYT?

adelinowona · 2025-08-07T19:54:37Z

src/MongoDB.Driver/AbortTransactionOptions.cs

+    internal sealed class AbortTransactionOptions
+    {
+        public AbortTransactionOptions()
+        {}


no need for a new line for this?

I suppose we do not need the parameter less constructor at all here. Thank you for bringing attention to this.

adelinowona · 2025-08-07T19:54:50Z

src/MongoDB.Driver/CommitTransactionOptions.cs

+    internal sealed class CommitTransactionOptions
+    {
+        public CommitTransactionOptions()
+        {}


same no need for a new line?

I suppose we do not need the parameter less constructor at all here. Thank you for bringing attention to this.

sanych-sun requested a review from a team as a code owner August 1, 2025 18:21

sanych-sun requested review from papafe and removed request for a team August 1, 2025 18:21

sanych-sun added the feature label Aug 1, 2025

sanych-sun requested review from adelinowona and removed request for papafe August 1, 2025 18:21

sanych-sun force-pushed the csharp3552 branch from 18a3762 to ebfedb6 Compare August 5, 2025 18:22

CSHARP-3552: CSOT: Transactions

e53816b

sanych-sun force-pushed the csharp3552 branch from ebfedb6 to e53816b Compare August 5, 2025 18:53

sanych-sun requested a review from rstam August 6, 2025 18:56

rstam requested changes Aug 7, 2025

View reviewed changes

adelinowona reviewed Aug 7, 2025

View reviewed changes

Oleksandr Poliakov added 2 commits August 7, 2025 14:43

Pr

ba9e276

Pr

061e04f

sanych-sun requested review from rstam and adelinowona August 7, 2025 22:06

CSHARP-3552: CSOT: Transactions #1745

Are you sure you want to change the base?

CSHARP-3552: CSOT: Transactions #1745

Conversation

sanych-sun commented Aug 1, 2025

Uh oh!

rstam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adelinowona left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!