CSHARP-5608: CSOT: Command Execution #1716

sanych-sun · 2025-06-23T19:07:56Z

No description provided.

Copilot

Pull Request Overview

This PR introduces explicit handling of server round-trip time ("RTT") by expanding SelectServer to return both a server and its RTT, and thread this RTT through various APIs and wire protocols to support operation timeouts and fail points.

Update SelectServer and SelectServerAsync to return (IServer server, TimeSpan roundTripTime).
Add serverRoundTripTime parameters to FailPoint.Configure, binding constructors, and CommandWireProtocol.
Inject RTT into command message sections for dynamic timeout calculation and update numerous tests to use the new overloads.

Reviewed Changes

Copilot reviewed 103 out of 103 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
tests/MongoDB.Driver.Tests/UnifiedTestOperations/UnifiedTargetedFailPointOperation.cs	Destructure `(server, roundTripTime)` and pass RTT into `FailPoint.Configure`
src/MongoDB.Driver/Core/WireProtocol/CommandUsingCommandMessageWireProtocol.cs	Thread `serverRoundTripTime` into command creation and timeout logic
src/MongoDB.Driver/OperationContextExtensions.cs	Add `HasOperationTimeout` and root context lookup
src/MongoDB.Driver/Core/WireProtocol/CommandWireProtocol.cs	Update `IWireProtocol` methods to accept `OperationContext` and RTT
tests/MongoDB.Driver.Tests/Core/Bindings/SingleServerReadWriteBindingTests.cs	Introduce RTT parameter in test constructors and add validation

Comments suppressed due to low confidence (3)

src/MongoDB.Driver/Core/WireProtocol/CommandWireProtocol.cs:88

[nitpick] The field _serverRoundTripTime is declared without XML doc. Add a brief comment explaining that this RTT is used for dynamic timeout calculations in command messages.

                messageEncoderSettings,

src/MongoDB.Driver/Core/WireProtocol/CommandUsingCommandMessageWireProtocol.cs:381

OperationContext does not define a RemainingTimeout property. You should compute the remaining time from the original timeout and elapsed time, or add a helper on OperationContext to expose the remaining timeout.

                var serverTimeout = operationContext.RemainingTimeout - _serverRoundTripTime;

src/MongoDB.Driver/Core/WireProtocol/CommandUsingCommandMessageWireProtocol.cs:630

RemainingTimeout is not a member of OperationContext. Consider replacing this check with your new helper (e.g., HasOperationTimeout) or implement a proper RemainingTimeout getter.

            if (operationContext.RemainingTimeout == Timeout.InfiniteTimeSpan)

adelinowona

No major comments from me, just minor issues/questions

src/MongoDB.Driver/OperationContextExtensions.cs

src/MongoDB.Driver/Core/WireProtocol/CommandUsingCommandMessageWireProtocol.cs

sanych-sun · 2025-06-25T00:32:45Z

src/MongoDB.Driver/Core/Clusters/Cluster.cs

-                        EndServerSelection(expirableClusterDescription.ClusterDescription, selector, result.ServerDescription, stopwatch);
-                        return result.Server;
+                        EndServerSelection(expirableClusterDescription.ClusterDescription, selector, description, stopwatch);
+                        return (server, description.AverageRoundTripTime);


Accordingly to CSOT spec we should use MinRoundTripTime here, but CSharp driver do not track it yet, we have only AverageRoundTripTime. We decided to go with the AverageRoundTripTime and replace it on the later stages of CSOT implementation. More over there is a chance that RTT will be removed from maxTimeMs calculation at all.
Find more details in the https://jira.mongodb.org/browse/CSHARP-5627

adelinowona

LGTM

sanych-sun · 2025-06-25T23:42:45Z

src/MongoDB.Driver/OperationContext.cs

@@ -61,10 +64,45 @@ public TimeSpan RemainingTimeout
            }
        }

+        [Obsolete("Do not use this property, unless it's needed to avoid breaking changes in public API")]
+        public CancellationToken CombinedCancellationToken


We have a number of use-cases when we cannot update the interfaces, because they are public (I've run into at least 2 cases like that: IStreamFactory and ISaslStep), so we have to squeeze both: cancellation token itself and timeout into linked cancellation token. Having such CombinedCancellationToken on operation context allow us to reuse instantiated CancellationTokenSources. Also I've marked the property as obsolete, so we easily can find all usages and replace with some interface changes in next major release.

Is [Obsolete] really useful here?

When this property is removed the compiler will tell us where it is used.

Yep, as I do not want to encourage more usages of the property.

sanych-sun · 2025-06-25T23:53:52Z

src/MongoDB.Driver/OperationContext.cs

@@ -94,7 +115,7 @@ public void WaitTask(Task task)
            }

            var timeout = RemainingTimeout;
-            if (timeout != System.Threading.Timeout.InfiniteTimeSpan && timeout < TimeSpan.Zero)


RemainingTimeout returns TimeSpan.Zero if timed out now, so the check could be simplified.

sanych-sun · 2025-06-25T23:55:02Z

src/MongoDB.Driver/OperationContext.cs

        {
-            var remainingTimeout = RemainingTimeout;
-            if (remainingTimeout == System.Threading.Timeout.InfiniteTimeSpan)


RemainingTimeout returns TimeSpan.Zero if timed out now, so the check could be simplified.

adelinowona

LGTM!

rstam · 2025-06-27T15:03:42Z

src/MongoDB.Driver/OperationContext.cs

@@ -61,10 +64,45 @@ public TimeSpan RemainingTimeout
            }
        }

+        [Obsolete("Do not use this property, unless it's needed to avoid breaking changes in public API")]
+        public CancellationToken CombinedCancellationToken


Is [Obsolete] really useful here?

When this property is removed the compiler will tell us where it is used.

rstam · 2025-06-30T13:58:35Z

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

-        {
-            get { return _reference.Instance.Session; }
-        }
+        public TimeSpan RoundTripTime => _reference.Instance.RoundTripTime;


Not needed if we use ISelectedServer interface.

In fact this file probably needs NO changes.

Reverted most of the changes.

rstam · 2025-06-30T13:59:55Z

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

@@ -27,25 +27,21 @@ internal sealed class ChannelReadBinding : IReadBinding
        private bool _disposed;
        private readonly ReadPreference _readPreference;
        private readonly IServer _server;
+        private readonly TimeSpan _roundTripTime;


This file needs NO changes if we use the new ISelectedServer interface.

rstam · 2025-06-30T14:00:17Z

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

@@ -26,24 +26,20 @@ internal sealed class ChannelReadWriteBinding : IReadWriteBinding
        private readonly IChannelHandle _channel;
        private bool _disposed;
        private readonly IServer _server;
+        private readonly TimeSpan _serverRoundTripTime;


This file needs NO changes if we use the new ISelectedServer interface.

rstam · 2025-06-30T14:00:43Z

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

@@ -72,7 +65,6 @@ public void Dispose()
            {
                _reference.DecrementReferenceCount();
                _disposed = true;
-                GC.SuppressFinalize(this);


Why did you remove this line?

As far as I know there is no reason to call GC.SuppressFinalize on class without finalizer.

rstam · 2025-06-30T14:30:46Z

src/MongoDB.Driver/Core/WireProtocol/CommandUsingCommandMessageWireProtocol.cs

@@ -49,6 +49,7 @@ internal sealed class CommandUsingCommandMessageWireProtocol<TCommandResult> : I
        private readonly CommandResponseHandling _responseHandling;
        private readonly IBsonSerializer<TCommandResult> _resultSerializer;
        private readonly ServerApi _serverApi;
+        private readonly TimeSpan _serverRoundTripTime;


Should name be _minRoundTripTime?

I prefer _roundTripTime, I suppose the fact that this is suppose to be Min is kind of secondary here. The main thing here - this is round trip time that need to be used in the calculation. Especially now, when we do not support MinRoundTripTime yet.

rstam · 2025-06-30T14:31:24Z

src/MongoDB.Driver/Core/WireProtocol/CommandUsingCommandMessageWireProtocol.cs

@@ -369,6 +376,12 @@ private Type0CommandMessageSection<BsonDocument> CreateType0Section(ConnectionDe
                }
            }

+            if (operationContext.IsOperationTimeoutConfigured())
+            {
+                var serverTimeout = operationContext.RemainingTimeout - _serverRoundTripTime;


We need to handle the possibility that serverTimeout will be negative here.

Yep. Good catch!

rstam · 2025-06-30T14:31:52Z

src/MongoDB.Driver/Core/WireProtocol/CommandWireProtocol.cs

@@ -45,6 +44,7 @@ internal sealed class CommandWireProtocol<TCommandResult> : IWireProtocol<TComma
        private readonly CommandResponseHandling _responseHandling;
        private readonly IBsonSerializer<TCommandResult> _resultSerializer;
        private readonly ServerApi _serverApi;
+        private readonly TimeSpan _serverRoundTripTime;


Should name be _minRoundTripTime?

rstam · 2025-06-30T14:32:51Z

src/MongoDB.Driver/Core/ChannelPinningHelper.cs

@@ -32,6 +32,7 @@ public static IReadBindingHandle CreateReadBinding(IClusterInternal cluster, ICo
            {
                readBinding = new ChannelReadWriteBinding(
                    session.CurrentTransaction.PinnedServer,
+                    session.CurrentTransaction.PinnedServerRoundTripTime,


This file needs NO changes if we use the new ISelectedServer interface.

rstam · 2025-06-30T14:33:29Z

src/MongoDB.Driver/GridFS/GridFSBucket.cs

@@ -1003,33 +1003,33 @@ private IReadBindingHandle GetSingleServerReadBinding(OperationContext operation
        {
            var readPreference = _options.ReadPreference ?? _database.Settings.ReadPreference;
            var selector = new ReadPreferenceServerSelector(readPreference);
-            var server = _cluster.SelectServer(operationContext, selector);
-            var binding = new SingleServerReadBinding(server, readPreference, NoCoreSession.NewHandle());
+            var (server, serverRoundTripTime) = _cluster.SelectServer(operationContext, selector);


This file needs NO changes if we use the new ISelectedServer interface.

rstam · 2025-07-02T21:13:41Z

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

-        {
-            get { return _server; }
-        }
+        public IServer Server => _server;


This file now only has cosmetic changes.

Up to you whether they belong in this PR or not.

rstam · 2025-07-02T21:49:44Z

src/MongoDB.Driver/Core/Servers/Server.cs

@@ -172,8 +192,12 @@ public void Invalidate(string reasonInvalidated, TopologyVersion responseTopolog

        public abstract void RequestHeartbeat();

-        // protected methods
+        public void ReturnConnection(IConnectionHandle connection)


We're not actually returning the connection at all here. The connection parameter is ignored.

I suggest naming this method DecrementOutstandingOperationsCount.

rstam · 2025-07-02T22:02:24Z

src/MongoDB.Driver/Core/Servers/ServerChannel.cs

+        private readonly IConnectionHandle _connection;
+        private readonly IServer _server;
+        private readonly InterlockedInt32 _state;
+        private readonly bool _ownConnection;


Keep the original name for this field: _decrementOperationsCount

rstam · 2025-07-02T22:03:16Z

src/MongoDB.Driver/Core/Servers/IServer.cs


        IChannelHandle GetChannel(OperationContext operationContext);
        Task<IChannelHandle> GetChannelAsync(OperationContext operationContext);
+        void HandleChannelException(IConnectionHandle connection, Exception exception);
+        void ReturnConnection(IConnectionHandle connection);


Name this method DecrementOutstandingOperationsCount.

A future refactoring might improve how we keep track of the operations count and might result in this method not being necessary in the future.

rstam

LGTM

rstam · 2025-07-02T22:48:47Z

src/MongoDB.Driver/Core/Servers/ServerChannel.cs

+            {
+                if (_decrementOperationsCount)
+                {
+                    _server.DecrementOutstandingOperationsCount();


Github PR doesn't show what this line used to be because it was in a different file, but with the latest changes this line changed from:

Interlocked.Decrement(ref _server._outstandingOperationsCount);

to this.

Now it is much clearer that we are preserving existing behavior!

sanych-sun requested a review from a team as a code owner June 23, 2025 19:07

sanych-sun added the feature label Jun 23, 2025

sanych-sun requested review from papafe, rstam and adelinowona and removed request for a team and papafe June 23, 2025 19:07

BorisDog requested a review from Copilot June 24, 2025 21:52

Copilot AI reviewed Jun 24, 2025

View reviewed changes

adelinowona requested changes Jun 25, 2025

View reviewed changes

sanych-sun commented Jun 25, 2025

View reviewed changes

sanych-sun requested a review from adelinowona June 25, 2025 17:40

adelinowona approved these changes Jun 25, 2025

View reviewed changes

sanych-sun requested a review from adelinowona June 25, 2025 23:38

sanych-sun commented Jun 25, 2025

View reviewed changes

adelinowona approved these changes Jun 27, 2025

View reviewed changes

rstam requested changes Jun 30, 2025

View reviewed changes

sanych-sun force-pushed the csharp5608 branch from 904ef77 to 2d06854 Compare June 30, 2025 20:36

CSHARP-5608: CSOT: Command Execution

7e00c43

sanych-sun force-pushed the csharp5608 branch from 2d06854 to 7e00c43 Compare July 1, 2025 00:02

sanych-sun requested a review from rstam July 1, 2025 00:03

Oleksandr Poliakov added 2 commits July 2, 2025 12:32

PR

1db6d65

PR

e723254

rstam requested changes Jul 2, 2025

View reviewed changes

PR

d5004eb

sanych-sun requested a review from rstam July 2, 2025 22:40

rstam approved these changes Jul 2, 2025

View reviewed changes

fix tests

18f01ef

sanych-sun merged commit f2382e6 into mongodb:main Jul 2, 2025
32 of 35 checks passed

sanych-sun deleted the csharp5608 branch July 2, 2025 23:43

CSHARP-5608: CSOT: Command Execution #1716

CSHARP-5608: CSOT: Command Execution #1716

Uh oh!

Conversation

sanych-sun commented Jun 23, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

adelinowona left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

sanych-sun Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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

sanych-sun Jun 25, 2025 •

edited

Loading