CSHARP-3662: MongoClientSettings.SocketTimeout does not work for values under 500ms on Windows for sync code #1690

sanych-sun · 2025-05-09T22:40:40Z

No description provided.

…es under 500ms on Windows for sync code

sanych-sun · 2025-05-09T22:56:03Z

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

+            }
+            catch (ObjectDisposedException ex)
+            {
+                throw new EndOfStreamException("The connection was interrupted.", ex);


Have to catch ObjectDisposedException and throw something that will make WrapExceptionIfRequired to throw MongoConnectionException. Otherwise we will fail to re-try on connection pool closing in-use connections.

How would we ever reach this line?

Any ObjectDisposedException thrown by line 50 is discarded by line 55.

This is another code path. We could get ObjectDisposedException here when connection pool was stopped with close in-use connection option. In such case connection pool will dispose all connections.

I've re-wrote the code to avoid the confusion.

BorisDog · 2025-05-20T17:13:30Z

src/MongoDB.Driver/Core/Connections/TcpStreamFactory.cs

+                try
+                {
+                    socket.Dispose();
+                    await connectTask.ConfigureAwait(false);


This probably deserves a comment, as it might be counter-intuitive. Same for sync version.

Will add the comment. But the reason to await here is to catch the ObjectDisposedException to avoid triggering UnobservedException.

Comments were added.

BorisDog · 2025-05-20T19:55:50Z

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

-                }
-                catch when (state >= 3)
+                var readOperation = stream.BeginRead(buffer, offset, count, null, null);
+                WaitHandle.WaitAny([readOperation.AsyncWaitHandle, cancellationToken.WaitHandle], timeout);


Evaluate the option of using ManualResetEventSlim instead.

BorisDog · 2025-05-20T22:58:56Z

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

+                try
+                {
+                    stream.Dispose();
+                    await readTask.ConfigureAwait(false);


Can waiting for readTask be blocking for long and cause significant deviation from timeout?

It should not. Because stream.Dispose closes socket, what should abort the task.

BorisDog · 2025-05-21T17:47:47Z

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

@@ -204,7 +242,7 @@ public static void WriteBytes(this Stream stream, IByteBuffer buffer, int offset
                cancellationToken.ThrowIfCancellationRequested();
                var backingBytes = buffer.AccessBackingBytes(offset);
                var bytesToWrite = Math.Min(count, backingBytes.Count);
-                stream.Write(backingBytes.Array, backingBytes.Offset, bytesToWrite); // TODO: honor cancellationToken?
+                stream.Write(backingBytes.Array, backingBytes.Offset, bytesToWrite, timeout, cancellationToken); // TODO: honor cancellationToken?


Remove the TODO comment

BorisDog · 2025-05-21T17:49:32Z

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


        public static async Task WriteAsync(this Stream stream, byte[] buffer, int offset, int count, TimeSpan timeout, CancellationToken cancellationToken)
        {
-            var state = 1; // 1 == writing, 2 == done writing, 3 == timedout, 4 == cancelled
+            var timeoutTask = Task.Delay(timeout);
+            var writeTask = stream.WriteAsync(buffer, offset, count, cancellationToken);


cancellationToken is passed in WriteAsync but not in ReadAsync

BorisDog · 2025-05-21T18:20:05Z

tests/MongoDB.Driver.Tests/Core/Connections/BinaryConnectionTests.cs

+                        .Returns(task);
+                    mockStream
+                        .Setup(s => s.EndRead(It.IsAny<IAsyncResult>()))
+                        .Returns<IAsyncResult>(x => ((Task<int>)x).GetAwaiter().GetResult());


Good opportunity to switch to Record.ExceptionAsync below.

BorisDog · 2025-05-21T18:22:22Z

tests/MongoDB.Driver.Tests/Core/Connections/BinaryConnectionTests.cs

-                        .Setup(s => s.Read(It.IsAny<byte[]>(), It.IsAny<int>(), It.IsAny<int>()))
-                        .Throws(new SocketException());
+                        .Setup(s => s.BeginRead(It.IsAny<byte[]>(), It.IsAny<int>(), It.IsAny<int>(), null, null))
+                        .Returns(task);


Does this mimic BeginRead that is implemented with Tasks?
If so, it that's not probably case for NetworkStream, at least for older frameworks.

Are we ok with loosing .Throws(new SocketException()); test case?

It will throw the same SocketException. Because the task was created from the exception. See above.

BorisDog · 2025-05-21T18:23:45Z

tests/MongoDB.Driver.Tests/Core/Connections/BinaryConnectionTests.cs

+                Exception exception;
+                if (async)
+                {
+                    exception = Record.Exception(() => _subject.ReceiveMessageAsync(1, encoderSelector, _messageEncoderSettings, CancellationToken.None).GetAwaiter().GetResult());


ExceptionAsync ?

rstam · 2025-05-22T15:00:28Z

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

+            }
+            catch (ObjectDisposedException ex)
+            {
+                throw new EndOfStreamException("The connection was interrupted.", ex);


How would we ever reach this line?

Any ObjectDisposedException thrown by line 50 is discarded by line 55.

rstam · 2025-05-22T15:06:14Z

src/MongoDB.Driver/Core/Compression/SnappyCompressor.cs

@@ -34,7 +34,7 @@ public void Compress(Stream input, Stream output)
        {
            var uncompressedSize = (int)(input.Length - input.Position);
            var uncompressedBytes = new byte[uncompressedSize]; // does not include uncompressed message headers
-            input.ReadBytes(uncompressedBytes, offset: 0, count: uncompressedSize, CancellationToken.None);
+            input.ReadBytes(uncompressedBytes, offset: 0, count: uncompressedSize, Timeout.InfiniteTimeSpan, CancellationToken.None);


Curious why this change is needed.

Because this code uses helper method that I've adjusted with new parameter, so I should pass something from here too.

Will stream.ReadTimeout still be respected by BeginRead?

As far as I understood Compress/Decompress works only with in-memory kind of streams. Not sure if timeout makes sense in such case.

rstam · 2025-05-22T15:07:39Z

src/MongoDB.Driver/Core/Connections/BinaryConnection.cs

@@ -333,14 +333,15 @@ private IByteBuffer ReceiveBuffer(CancellationToken cancellationToken)
            try
            {
                var messageSizeBytes = new byte[4];
-                _stream.ReadBytes(messageSizeBytes, 0, 4, cancellationToken);
+                var readTimeout = _stream.CanTimeout ? TimeSpan.FromMilliseconds(_stream.ReadTimeout) : Timeout.InfiniteTimeSpan;


Seems weird to be getting the readTimeout from the Stream. Shouldn't it come from settings somewhere?

Also, why do we care about _stream.CanTimeout? The new code will support timeouts on any Stream no matter what the value of _stream.CanTimeout is.

This code is inhereted/copied from the RecieveBufferAsync. I can double-check when/which streams could have CanTimeout = false.

I agree that _stream.CanTimeout seems irrelevant here.

Ok. Should I remove checking the _stream.CanTimeout for both sync and async versions? Async version used to have that check before.

Yes, I think we can ignore this now as we are doing our own timeouts.

rstam · 2025-05-22T15:08:04Z

src/MongoDB.Driver/Core/Connections/BinaryConnection.cs

@@ -535,7 +536,8 @@ private void SendBuffer(IByteBuffer buffer, CancellationToken cancellationToken)

                try
                {
-                    _stream.WriteBytes(buffer, 0, buffer.Length, cancellationToken);
+                    var writeTimeout = _stream.CanTimeout ? TimeSpan.FromMilliseconds(_stream.WriteTimeout) : Timeout.InfiniteTimeSpan;


Similar question here as for readTimeout above.

JamesKovacs

Reasonable changes. My major concern is accessing objects after disposal, which happens in a few places. This will typically raise an ObjectDisposedException. Are these calls reversed intentionally (e.g. dispose then a further cleanup call) and if so why?

JamesKovacs · 2025-05-22T21:52:53Z

src/MongoDB.Driver/Core/Connections/TcpStreamFactory.cs

+                try
+                {
+                    socket.Dispose();
+                    socket.EndConnect(connectOperation);


Shouldn't these calls be reversed? You shouldn't be calling methods on an object after disposal.

I've removed EndConnect call. We do have to await on task returned for async code path to observe the exception. I was under impression that same logic is applicable for Begin/End approach, but it is not.

JamesKovacs · 2025-05-22T21:53:48Z

src/MongoDB.Driver/Core/Connections/TcpStreamFactory.cs

+                try
+                {
+                    socket.Dispose();
+                    await connectTask.ConfigureAwait(false);


JamesKovacs · 2025-05-22T21:57:07Z

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

+                    try
+                    {
+                        stream.Dispose();
+                        stream.EndRead(readOperation);


Same comment here about using an object after disposal.

BorisDog · 2025-05-23T23:58:15Z

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

            {
-                try
+                using var manualResetEvent = new ManualResetEventSlim();
+                readOperation = stream.BeginRead(buffer, offset, count, state =>


Consider placing each argument in a separate line.
Also just state => ((ManualResetEventSlim)state.AsyncState).Set() is sufficient.

sanych-sun · 2025-05-28T19:50:36Z

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

+
+namespace MongoDB.Driver.Core.Misc
+{
+    public class TaskExtensionsTests


This test collection testing our pollyfil of Task.WaitAsync for net472 and netstandard2.1. Technically it do not have to run for net6, but I decided to run it for now, to demonstrate that our implementation behave the same way as native.

BorisDog · 2025-05-28T19:57:08Z

src/MongoDB.Driver/Core/Connections/BinaryConnection.cs

@@ -333,14 +333,15 @@ private IByteBuffer ReceiveBuffer(CancellationToken cancellationToken)
            try
            {
                var messageSizeBytes = new byte[4];
-                _stream.ReadBytes(messageSizeBytes, 0, 4, cancellationToken);
+                var readTimeout = _stream.CanTimeout ? TimeSpan.FromMilliseconds(_stream.ReadTimeout) : Timeout.InfiniteTimeSpan;


I agree that _stream.CanTimeout seems irrelevant here.

BorisDog · 2025-05-28T20:10:12Z

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

+            }
+            catch
+            {
+                // ignore any exceptions


nit: Capital i for consistency.

BorisDog · 2025-05-28T20:12:59Z

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

+                await HandleTaskCancellation().ConfigureAwait(false);
+                throw;
+            }
+            catch (TimeoutException)


why not: catch (Exception e) when (e is OperationCanceledException || e is TimeoutException)
And no need for HandleTaskCancellation then.

I was about to propose the same thing, slightly differently:

catch (Exception ex) when (ex is OperationCanceledException or TimeoutException)

BorisDog · 2025-05-28T20:14:08Z

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

+            var readTask = stream.ReadAsync(buffer, offset, count);
+            try
+            {
+                return await readTask.WaitAsync(timeout, cancellationToken).ConfigureAwait(false);


BorisDog · 2025-05-28T20:15:49Z

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

            {
-                try
+                using var manualResetEvent = new ManualResetEventSlim();
+                writeOperation = stream.BeginWrite(buffer, offset, count, state =>


Not need for brackets:
writeOperation = stream.BeginWrite(buffer, offset, count, state => ((ManualResetEventSlim)state.AsyncState).Set(), manualResetEvent);

BorisDog · 2025-05-28T20:16:26Z

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

-            var bytesRead = 0;
-            using (new Timer(_ => ChangeState(3), null, timeout, Timeout.InfiniteTimeSpan))
-            using (cancellationToken.Register(() => ChangeState(4)))
+            IAsyncResult readOperation;


Can define readOperation inside the scope (write as well)?
var readOperation = ...

BorisDog · 2025-05-28T20:19:25Z

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

                }
-                catch when (state == 1)
+                catch (OperationCanceledException)


Would handling + in the outer catch be readable/result in shorter code?

BorisDog · 2025-05-28T20:21:38Z

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

+            }
+            catch (ObjectDisposedException ex)
+            {
+                throw new EndOfStreamException("The connection was interrupted.", ex);


So we don't have EndOfStreamException in async case?

Good question. Will double-check.

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

BorisDog · 2025-05-28T20:25:42Z

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

+            }
+        }
+
+        #endif


minor: Add and empty line after endif?

And remove the empty line before it?

rstam · 2025-05-28T21:49:37Z

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

+                await HandleTaskCancellation().ConfigureAwait(false);
+                throw;
+            }
+            catch (TimeoutException)


I was about to propose the same thing, slightly differently:

catch (Exception ex) when (ex is OperationCanceledException or TimeoutException)

rstam · 2025-05-28T21:50:45Z

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

+                await HandleTaskCancellation().ConfigureAwait(false);
+                throw;
+            }
+            catch (TimeoutException)


Same comment as for ReadAsync:

catch (Exception ex) when (ex is OperationCanceledException or TimeoutException)

rstam · 2025-05-28T22:00:38Z

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

+
+        public static async Task WaitAsync(this Task task, TimeSpan timeout, CancellationToken cancellationToken)
+        {
+            ValidateTimeout(timeout);


These extension methods are nice!

But there is a subtle issue here that we may or may not want to address.

The way this is currently coded when passing an invalid timeout the exception will NOT be thrown immediately. Instead a Task will be returned and the exception will be thrown later and observed by the code that awaits the Task.

To immediately throw an exception without returning a Task requires some tricks involving an async helper method.

I leave it up to you to decide if you want to be more precise about when the exception is thrown.

For one example of how to throw an exception immediately instead of returning a Task see:

https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/[Task.cs](https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs,2825),2825

See also:

https://learn.microsoft.com/en-us/dotnet/standard/exceptions/best-practices-for-exceptions#throw-argument-validation-exceptions-synchronously

Which in turn links to:

https://learn.microsoft.com/en-us/dotnet/csharp/fundamentals/exceptions/creating-and-throwing-exceptions#exceptions-in-task-returning-methods

which has an example of using a static local method to separate the sync argument checking from the async part that does the work. This looks like a nice approach.

BorisDog · 2025-05-28T21:56:16Z

src/MongoDB.Driver/Core/Connections/BinaryConnection.cs

@@ -333,14 +333,15 @@ private IByteBuffer ReceiveBuffer(CancellationToken cancellationToken)
            try
            {
                var messageSizeBytes = new byte[4];
-                _stream.ReadBytes(messageSizeBytes, 0, 4, cancellationToken);
+                var readTimeout = _stream.CanTimeout ? TimeSpan.FromMilliseconds(_stream.ReadTimeout) : Timeout.InfiniteTimeSpan;


Yes, I think we can ignore this now as we are doing our own timeouts.

BorisDog · 2025-05-28T22:00:57Z

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

+
+            await task.WaitAsync(Timeout.InfiniteTimeSpan, CancellationToken.None);
+
+            task.IsCompleted.Should().BeTrue();


I think I understand why you duplicated this test, but I think it's ok to just have one:
WaitAsync_should_work_for_task_with_inifite_timeout.

rstam · 2025-05-28T22:03:47Z

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

+
+        public static async Task WaitAsync(this Task task, TimeSpan timeout, CancellationToken cancellationToken)
+        {
+            ValidateTimeout(timeout);


See also:

https://learn.microsoft.com/en-us/dotnet/standard/exceptions/best-practices-for-exceptions#throw-argument-validation-exceptions-synchronously

rstam · 2025-05-28T22:04:58Z

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

+
+        public static async Task WaitAsync(this Task task, TimeSpan timeout, CancellationToken cancellationToken)
+        {
+            ValidateTimeout(timeout);


Which in turn links to:

https://learn.microsoft.com/en-us/dotnet/csharp/fundamentals/exceptions/creating-and-throwing-exceptions#exceptions-in-task-returning-methods

which has an example of using a static local method to separate the sync argument checking from the async part that does the work. This looks like a nice approach.

rstam · 2025-05-28T22:07:51Z

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

+            }
+        }
+
+        #endif


And remove the empty line before it?

rstam · 2025-05-28T22:08:35Z

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

+            throw new TimeoutException();
+        }
+
+        private static void ValidateTimeout(TimeSpan timeout)


You can keep this name if you want, but normally we would have named a method like this EnsureTimeoutIsValid.

CSHARP-3662: MongoClientSettings.SocketTimeout does not work for valu…

e60210b

…es under 500ms on Windows for sync code

sanych-sun requested a review from a team as a code owner May 9, 2025 22:40

sanych-sun added the bug label May 9, 2025

sanych-sun requested review from papafe, BorisDog and JamesKovacs and removed request for a team and papafe May 9, 2025 22:40

sanych-sun commented May 9, 2025

View reviewed changes

BorisDog requested a review from rstam May 20, 2025 17:20

BorisDog reviewed May 20, 2025

View reviewed changes

BorisDog requested changes May 21, 2025

View reviewed changes

rstam requested changes May 22, 2025

View reviewed changes

Oleksandr Poliakov added 2 commits May 22, 2025 21:29

PR

ea9d7d1

Fix tests

f276888

sanych-sun requested review from BorisDog and rstam May 23, 2025 16:59

JamesKovacs requested changes May 23, 2025

View reviewed changes

BorisDog reviewed May 23, 2025

View reviewed changes

Oleksandr Poliakov added 3 commits May 27, 2025 14:12

PR

e7095f9

pr

fe81697

Use Task.WaitAsync on async code-path.

8f3f0ac

sanych-sun commented May 28, 2025

View reviewed changes

sanych-sun requested review from BorisDog and JamesKovacs May 28, 2025 19:50

BorisDog requested changes May 28, 2025

View reviewed changes

rstam requested changes May 28, 2025

View reviewed changes

BorisDog requested changes May 28, 2025

View reviewed changes

rstam requested changes May 28, 2025

View reviewed changes

+                          }
+                      }
+                      #endif


		await task.WaitAsync(Timeout.InfiniteTimeSpan, CancellationToken.None);

		task.IsCompleted.Should().BeTrue();

CSHARP-3662: MongoClientSettings.SocketTimeout does not work for values under 500ms on Windows for sync code #1690

Are you sure you want to change the base?

CSHARP-3662: MongoClientSettings.SocketTimeout does not work for values under 500ms on Windows for sync code #1690

Conversation

sanych-sun commented May 9, 2025

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!

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!

JamesKovacs 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