Skip to content

CSHARP-5618: CSOT: Race condition in ExclusiveConnectionPool could lead to ArgumentOutOfRangeException #1712

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sanych-sun merged 3 commits into from
Jun 18, 2025
Merged

CSHARP-5618: CSOT: Race condition in ExclusiveConnectionPool could lead to ArgumentOutOfRangeException #1712

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3cf14d6
CSHARP-5618: Race condition in ExclusiveConnectionPool could lead to …
sanych-sun Jun 17, 2025
f1b6a98
PR
sanych-sun Jun 17, 2025
83e8d4d
PR
sanych-sun Jun 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions src/MongoDB.Driver/OperationContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,13 @@ public TimeSpan RemainingTimeout
return System.Threading.Timeout.InfiniteTimeSpan;
}

return Timeout - Stopwatch.Elapsed;
var result = Timeout - Stopwatch.Elapsed;
Copy link
Member Author

@sanych-sun sanych-sun Jun 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many .net APIs that accepts timeouts validates the timeout and throws ArgumentException on negative values (for example SemaphoreSlim). On other hand if it's TimeSpan.Zero it just considered as timeout out. So it's safer to use TimeSpan.Zero for timed out contexts.

if (result < TimeSpan.Zero)
{
result = TimeSpan.Zero;
}

return result;
}
}

Expand All @@ -69,7 +75,7 @@ public bool IsTimedOut()
return false;
}

return remainingTimeout < TimeSpan.Zero;
return remainingTimeout == TimeSpan.Zero;
}

public void ThrowIfTimedOutOrCanceled()
Expand Down Expand Up @@ -141,6 +147,11 @@ public async Task WaitTaskAsync(Task task)

public OperationContext WithTimeout(TimeSpan timeout)
{
if (timeout != System.Threading.Timeout.InfiniteTimeSpan && timeout < TimeSpan.Zero)
{
throw new ArgumentOutOfRangeException(nameof(timeout), timeout, "The timeout must represent a value between -1 and Int32.MaxValue, inclusive.");
}

var remainingTimeout = RemainingTimeout;
if (timeout == System.Threading.Timeout.InfiniteTimeSpan)
{
Expand All @@ -151,6 +162,11 @@ public OperationContext WithTimeout(TimeSpan timeout)
timeout = remainingTimeout;
}

if (timeout == TimeSpan.Zero)
{
throw new TimeoutException();
Copy link
Contributor

@BorisDog BorisDog Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure about throwing here, can't we create timed out contexts?? Feels like throwing should be part of some explicit validation like ThrowIfTimedOutOrCanceled.

Copy link
Member Author

@sanych-sun sanych-sun Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly not sure. Probably you are right, as throwing here might be not-expected, but throwing once we decide to Wait on something - will be more expected behavior.
Will revert.

Copy link
Member Author

@sanych-sun sanych-sun Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted.

}

return new OperationContext(timeout, CancellationToken)
{
ParentContext = this
Expand Down
2 changes: 1 addition & 1 deletion tests/MongoDB.Bson.TestHelpers/Threading/ThreadingUtilities.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public static void ExecuteOnNewThreads(int threadsCount, Action<int> action, int

if (exceptions.Any())
{
throw exceptions.First();
throw new AggregateException(exceptions);
Copy link
Member Author

@sanych-sun sanych-sun Jun 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throwing like it was before messes up the call stack, when AggregateException can preserve the original stack trace of InnerException.

}
}

Expand Down
31 changes: 24 additions & 7 deletions tests/MongoDB.Driver.Tests/OperationContextTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,16 +68,12 @@ public void RemainingTimeout_should_return_infinite_for_infinite_timeout()
}

[Fact]
public void RemainingTimeout_could_be_negative()
public void RemainingTimeout_should_return_zero_for_timeout_context()
{
var timeout = TimeSpan.FromMilliseconds(5);
var stopwatch = Stopwatch.StartNew();
var operationContext = new OperationContext(TimeSpan.FromMilliseconds(5), CancellationToken.None);
Thread.Sleep(10);
stopwatch.Stop();

var operationContext = new OperationContext(stopwatch, timeout, CancellationToken.None);

operationContext.RemainingTimeout.Should().Be(timeout - stopwatch.Elapsed);
operationContext.RemainingTimeout.Should().Be(TimeSpan.Zero);
}

[Theory]
Expand Down Expand Up @@ -276,6 +272,27 @@ public void WithTimeout_should_set_ParentContext()

resultContext.ParentContext.Should().Be(operationContext);
}

[Fact]
public void WithTimeout_throws_on_timed_out_context()
{
var operationContext = new OperationContext(TimeSpan.FromMilliseconds(5), CancellationToken.None);
Thread.Sleep(10);

var exception = Record.Exception(() => operationContext.WithTimeout(TimeSpan.FromSeconds(10)));
exception.Should().BeOfType<TimeoutException>();
}

[Fact]
public void WithTimeout_throws_on_negative_timeout()
{
var operationContext = new OperationContext(Timeout.InfiniteTimeSpan, CancellationToken.None);

var exception = Record.Exception(() => operationContext.WithTimeout(TimeSpan.FromSeconds(-5)));

exception.Should().BeOfType<ArgumentOutOfRangeException>()
.Subject.ParamName.Should().Be("timeout");
}
}
}

Loading