Skip to content

Commit

Permalink
only consider cancellation if it doesn't complete synchronously
Browse files Browse the repository at this point in the history
  • Loading branch information
mgravell committed Nov 5, 2020
1 parent 3d43209 commit e1e7189
Showing 1 changed file with 23 additions and 16 deletions.
39 changes: 23 additions & 16 deletions src/StackExchange.Redis/PhysicalConnection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,7 @@ public void Dispose()
}
OnCloseEcho();
_arena.Dispose();
_reusableFlushSyncTokenSource?.Dispose();
GC.SuppressFinalize(this);
}

Expand Down Expand Up @@ -872,31 +873,37 @@ private async ValueTask<WriteResult> FlushAsync_Awaited(PhysicalConnection conne
}
}

CancellationTokenSource _reusableFlushSyncTokenSource;
[Obsolete("this is an anti-pattern; work to reduce reliance on this is in progress")]
internal WriteResult FlushSync(bool throwOnFailure, int millisecondsTimeout)
{
using (var source = new CancellationTokenSource())
var cts = _reusableFlushSyncTokenSource ??= new CancellationTokenSource();
var flush = FlushAsync(throwOnFailure, cts.Token);
if (!flush.IsCompletedSuccessfully)
{
source.CancelAfter(TimeSpan.FromMilliseconds(millisecondsTimeout));
var flush = FlushAsync(throwOnFailure, source.Token);
if (!flush.IsCompletedSuccessfully)
// only schedule cancellation if it doesn't complete synchronously; at this point, it is doomed
_reusableFlushSyncTokenSource = null;
cts.CancelAfter(TimeSpan.FromMilliseconds(millisecondsTimeout));

This comment has been minimized.

Copy link
@Plasma

Plasma Nov 15, 2020

Do we need to ensure we dispose of the cts here after we decide to use it, because _reusableFlushSyncTokenSource gets set to NULL and so won't be disposed of in Dispose() on line 272.

This comment has been minimized.

Copy link
@mgravell

mgravell Nov 16, 2020

Author Collaborator

Yes, and we do - see L902

try
{
try
{
// here lies the evil
flush.AsTask().Wait();
}
catch (AggregateException ex)
// here lies the evil
flush.AsTask().Wait();
}
catch (AggregateException ex)
{
if (ex.InnerExceptions.Any(e => e is TaskCanceledException))
{
if (ex.InnerExceptions.Any(e => e is TaskCanceledException))
{
ThrowTimeout();
}
throw;
ThrowTimeout();
}
throw;
}
finally
{
cts.Dispose();
}
return flush.Result;
}
return flush.Result;

void ThrowTimeout()
{
#if DEBUG
Expand Down

0 comments on commit e1e7189

Please sign in to comment.