CSHARP-3549: CSOT: Add timeoutMS to settings #1721

sanych-sun · 2025-07-10T00:35:38Z

No description provided.

rstam

Overall looks good. See minor comments.

rstam · 2025-07-29T20:43:25Z

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

@@ -459,12 +460,12 @@ public static string IsNullOrNotEmpty(string value, string paramName)
        /// <returns>The value of the parameter.</returns>
        public static TimeSpan IsValidTimeout(TimeSpan value, string paramName)
        {
-            if (value < TimeSpan.Zero && value != Timeout.InfiniteTimeSpan)
+            if (value > TimeSpan.Zero || value == Timeout.InfiniteTimeSpan)


Why did you flip the logic in this method?

All the other methods end with return value.

Somehow this way it's easier to understand for me. But not critical, will revert to follow other methods pattern.

rstam · 2025-07-29T21:33:01Z

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

                {
-                    throw new TimeoutException();


Is it OK that we no longer throw an exception?

Yes, you are right. We should throw for negative timeouts. Will return the exception.

rstam · 2025-07-29T21:34:49Z

src/MongoDB.Driver/ClientSessionExtensions.cs


-namespace MongoDB.Driver
+internal static class ClientSessionExtensions


Should this file/class be called IClientSessionExtensions?

rstam · 2025-07-29T21:37:16Z

src/MongoDB.Driver/OperationContext.cs

@@ -87,7 +87,7 @@ public CancellationToken CombinedCancellationToken
        }
        private Stopwatch Stopwatch { get; }

-        public TimeSpan Timeout { get; }
+        public TimeSpan? Timeout { get; }


I find it really unhelpful when a class has BOTH fields and auto-properties.

I'm not a fan of auto-properties in general, but I'm specially not a fan of mixing fields and auto-properties in the same class. Mixing them makes it much harder to understand what state a class maintains.

rstam · 2025-07-29T21:53:41Z

src/MongoDB.Driver/MongoCollectionImpl.cs

@@ -114,13 +109,13 @@ public override IAsyncCursor<TResult> Aggregate<TResult>(IClientSessionHandle se
            if (isAggregateToCollection)
            {
                var aggregateOperation = CreateAggregateToCollectionOperation(renderedPipeline, options);
-                ExecuteWriteOperation(session, aggregateOperation, cancellationToken);
+                ExecuteWriteOperation(session, aggregateOperation, options.Timeout, cancellationToken);


Sometime I see options.Timeout and other times I see options?.Timeout.

Should they ALL have ??

Let me double-check. I think in some cases there is no-way to have null options.

Yep. There are number of cases when options is guaranteed to be not-null. In all other cases there is Null-conditional operators.

That's a pain. I leave it to you then to check that there are no missing ?.

rstam · 2025-07-29T21:54:57Z

src/MongoDB.Driver/MongoUrlBuilder.cs

@@ -296,7 +297,7 @@ public TimeSpan HeartbeatTimeout
            get { return _heartbeatTimeout; }
            set
            {
-                if (value < TimeSpan.Zero && value != Timeout.InfiniteTimeSpan)
+                if (value < TimeSpan.Zero && value != System.Threading.Timeout.InfiniteTimeSpan)


Why this change?

Now as MongoUrlBuilder has property named Timeout, we should use full name here.

That's unfortunate.

rstam

LGTM with minor comments

rstam · 2025-07-30T01:13:15Z

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

@@ -444,11 +444,12 @@ public static string IsNullOrNotEmpty(string value, string paramName)
        /// <returns>The value of the parameter.</returns>
        public static TimeSpan? IsNullOrValidTimeout(TimeSpan? value, string paramName)
        {
-            if (value != null)
+            if (value == null)


There was probably no need to touch this file at all.

I don't mind leaving these changes in. I think they improve this code a bit.

Maybe not...

In general I don't like making random changes to files that have nothing to do with the PR.

If we think these methods could be written in a better way we should make a ticket for it.

The reason that all methods end with return value is that in general an Ensure method might throw more than one exception, so the general pattern is:

check for invalid condition 1 and throw exception with suitable message

check for invalid condition 2 and throw exception with suitable message

...

return value

Same story here as with previous change in the file: some how "quick returns" works better for me to understand the code. But I do not mind to revert the code.
And technically this method is kind of related to the PR: we used to have unused method Ensure.IsValidTimeout. Instead of implementing new method I've decided to adopt it and was making sure the logic is appropriate for the usage. That's the reason why I touched the file.

Anyhow: reverted 🙂

rstam · 2025-07-30T01:22:14Z

src/MongoDB.Driver/MongoCollectionImpl.cs

@@ -114,13 +109,13 @@ public override IAsyncCursor<TResult> Aggregate<TResult>(IClientSessionHandle se
            if (isAggregateToCollection)
            {
                var aggregateOperation = CreateAggregateToCollectionOperation(renderedPipeline, options);
-                ExecuteWriteOperation(session, aggregateOperation, cancellationToken);
+                ExecuteWriteOperation(session, aggregateOperation, options.Timeout, cancellationToken);


That's a pain. I leave it to you then to check that there are no missing ?.

rstam · 2025-07-30T01:23:30Z

src/MongoDB.Driver/MongoUrlBuilder.cs

@@ -296,7 +297,7 @@ public TimeSpan HeartbeatTimeout
            get { return _heartbeatTimeout; }
            set
            {
-                if (value < TimeSpan.Zero && value != Timeout.InfiniteTimeSpan)
+                if (value < TimeSpan.Zero && value != System.Threading.Timeout.InfiniteTimeSpan)


That's unfortunate.

rstam · 2025-07-30T01:27:17Z

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

-                    throw new TimeoutException();
-                }
+                    serverTimeout -= _roundTripTime;
+                    if (serverTimeout < TimeSpan.Zero)


Should this be changed to <=?

Done, thank you!

adelinowona

LGTM! left a comment you can tackle if you think it's ok to do it now.

adelinowona · 2025-07-30T16:37:21Z

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

            var effectiveComment = _comment;
            var effectiveHint = _hint;
            var effectiveMax = _max;
-            var effectiveMaxTime = _maxTime;
            var effectiveMin = _min;
            var effectiveReturnKey = _returnKey;
            var effectiveShowRecordId = _showRecordId;


What's the point of all this effective variables? can't we just remove them while we are here?

adelinowona · 2025-07-30T16:40:53Z

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

-                    throw new TimeoutException();
-                }
+                    serverTimeout -= _roundTripTime;
+                    if (serverTimeout < TimeSpan.Zero)


adelinowona · 2025-07-30T17:57:22Z

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

@@ -444,11 +444,12 @@ public static string IsNullOrNotEmpty(string value, string paramName)
        /// <returns>The value of the parameter.</returns>
        public static TimeSpan? IsNullOrValidTimeout(TimeSpan? value, string paramName)
        {
-            if (value != null)
+            if (value == null)


I don't mind leaving these changes in. I think they improve this code a bit.

rstam · 2025-07-30T18:03:09Z

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

@@ -444,11 +444,12 @@ public static string IsNullOrNotEmpty(string value, string paramName)
        /// <returns>The value of the parameter.</returns>
        public static TimeSpan? IsNullOrValidTimeout(TimeSpan? value, string paramName)
        {
-            if (value != null)
+            if (value == null)


Maybe not...

In general I don't like making random changes to files that have nothing to do with the PR.

If we think these methods could be written in a better way we should make a ticket for it.

The reason that all methods end with return value is that in general an Ensure method might throw more than one exception, so the general pattern is:

check for invalid condition 1 and throw exception with suitable message

check for invalid condition 2 and throw exception with suitable message

...

return value

rstam · 2025-07-30T18:16:51Z

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

@@ -444,11 +444,12 @@ public static string IsNullOrNotEmpty(string value, string paramName)
        /// <returns>The value of the parameter.</returns>
        public static TimeSpan? IsNullOrValidTimeout(TimeSpan? value, string paramName)
        {
-            if (value != null)
+            if (value == null)


rstam

LGTM

sanych-sun requested review from BorisDog and adelinowona July 10, 2025 00:35

sanych-sun requested a review from a team as a code owner July 10, 2025 00:35

sanych-sun added the feature label Jul 10, 2025

sanych-sun marked this pull request as draft July 10, 2025 22:02

sanych-sun force-pushed the csharp3549 branch 2 times, most recently from 0942f85 to af44109 Compare July 19, 2025 01:35

sanych-sun marked this pull request as ready for review July 28, 2025 17:14

sanych-sun force-pushed the csharp3549 branch from 3655c7c to 7bd7584 Compare July 28, 2025 22:52

CSHARP-3549: CSOT: Add timeoutMS to settings

b4711fe

sanych-sun force-pushed the csharp3549 branch from 7bd7584 to b4711fe Compare July 29, 2025 02:19

pr

eb38209

rstam requested changes Jul 29, 2025

View reviewed changes

pr

51c7dc8

sanych-sun requested a review from rstam July 29, 2025 22:21

rstam approved these changes Jul 30, 2025

View reviewed changes

adelinowona previously approved these changes Jul 30, 2025

View reviewed changes

rstam requested changes Jul 30, 2025

View reviewed changes

Pr

e555c26

sanych-sun dismissed adelinowona’s stale review via e555c26 July 30, 2025 18:44

sanych-sun requested review from rstam and adelinowona July 30, 2025 18:49

adelinowona approved these changes Jul 30, 2025

View reviewed changes

rstam approved these changes Jul 30, 2025

View reviewed changes

sanych-sun merged commit a93f29e into mongodb:main Jul 30, 2025
33 of 35 checks passed

sanych-sun deleted the csharp3549 branch July 30, 2025 20:35


		namespace MongoDB.Driver
		internal static class ClientSessionExtensions

CSHARP-3549: CSOT: Add timeoutMS to settings #1721

CSHARP-3549: CSOT: Add timeoutMS to settings #1721

Uh oh!

Conversation

sanych-sun commented Jul 10, 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!

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!

sanych-sun Jul 30, 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!

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!

rstam left a comment

Choose a reason for hiding this comment

Uh oh!

sanych-sun Jul 30, 2025 •

edited

Loading