CSHARP-5618: CSOT: Race condition in ExclusiveConnectionPool could lead to ArgumentOutOfRangeException #1712
Conversation
…ArgumentOutOfRangeException
sanych-sun
requested review from
papafe and
adelinowona
and removed request for
a team and
papafe
| @@ -53,7 +53,13 @@ public TimeSpan RemainingTimeout | |||
| return System.Threading.Timeout.InfiniteTimeSpan; | |||
| } | |||
|
|
|||
| return Timeout - Stopwatch.Elapsed; | |||
| var result = Timeout - Stopwatch.Elapsed; | |||
There was a problem hiding this comment.
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.
| @@ -29,7 +29,7 @@ public static void ExecuteOnNewThreads(int threadsCount, Action<int> action, int | |||
|
|
|||
| if (exceptions.Any()) | |||
| { | |||
| throw exceptions.First(); | |||
| throw new AggregateException(exceptions); | |||
There was a problem hiding this comment.
throwing like it was before messes up the call stack, when AggregateException can preserve the original stack trace of InnerException.
sanych-sun
changed the title
| @@ -151,6 +162,11 @@ public OperationContext WithTimeout(TimeSpan timeout) | |||
| timeout = remainingTimeout; | |||
| } | |||
|
|
|||
| if (timeout == TimeSpan.Zero) | |||
| { | |||
| throw new TimeoutException(); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
No description provided.