-
Notifications
You must be signed in to change notification settings - Fork 1k
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.
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
Streams ensure cts cancel fix statecheck #6935
Changes from 5 commits
e33e50d
ca645fe
a668833
f475621
3c53c7c
ff6f7c0
106236a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3812,18 +3812,19 @@ private sealed class Logic : OutGraphStageLogic | |
private readonly Action<T> _onSuccess; | ||
private readonly Action<Exception> _onFailure; | ||
private readonly Action _onComplete; | ||
private readonly CancellationTokenSource _completionCts; | ||
|
||
private CancellationTokenSource _completionCts; | ||
private IAsyncEnumerator<T> _enumerator; | ||
|
||
public Logic(SourceShape<T> shape, IAsyncEnumerable<T> enumerable) : base(shape) | ||
{ | ||
|
||
_enumerable = enumerable; | ||
_outlet = shape.Outlet; | ||
_onSuccess = GetAsyncCallback<T>(OnSuccess); | ||
_onFailure = GetAsyncCallback<Exception>(OnFailure); | ||
_onComplete = GetAsyncCallback(OnComplete); | ||
|
||
_completionCts = new CancellationTokenSource(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Controversial possibly, but I felt it safer to move this to ctor. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call IMHO |
||
SetHandler(_outlet, this); | ||
} | ||
[MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
|
@@ -3838,9 +3839,33 @@ public Logic(SourceShape<T> shape, IAsyncEnumerable<T> enumerable) : base(shape) | |
public override void PreStart() | ||
{ | ||
base.PreStart(); | ||
_completionCts = new CancellationTokenSource(); | ||
_enumerator = _enumerable.GetAsyncEnumerator(_completionCts.Token); | ||
} | ||
|
||
public override void PostStop() | ||
{ | ||
try | ||
{ | ||
_completionCts.Cancel(); | ||
_completionCts.Dispose(); | ||
Comment on lines
+3849
to
+3850
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should ALWAYS be disposing the CTS. What is more interesting to ask, is whether it's worth us having some sort of flag to know whether we should bother canceling or not. I went this route because in theory, under happy-completion path nobody cares about this CTS anyway. That said there -may- be value in some cases of not calling cancel, but can't think of any. |
||
} | ||
catch(Exception ex) | ||
{ | ||
// This should never happen | ||
Log.Error(ex, "AsyncEnumerable threw while cancelling CancellationTokenSource"); | ||
} | ||
try | ||
{ | ||
var disposed = _enumerator.DisposeAsync().AsTask().Wait(TimeSpan.FromSeconds(10)); | ||
Arkatufus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if(!disposed) | ||
Log.Error("Failed to dispose underlying async enumerator. DisposeAsync timed out after 10 seconds."); | ||
} | ||
catch(Exception ex) | ||
{ | ||
Log.Error(ex, "Underlying async enumerator threw an exception while being disposed."); | ||
} | ||
base.PostStop(); | ||
} | ||
|
||
public override void OnPull() | ||
{ | ||
|
@@ -3859,26 +3884,12 @@ public override void OnPull() | |
// if result is false, it means enumerator was closed. Complete stage in that case. | ||
CompleteStage(); | ||
} | ||
} | ||
else if (vtask.IsCompleted) // IsCompleted covers Faulted, Cancelled, and RanToCompletion async state | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is racy, so removed. Exception case is not as big a deal anyway. |
||
{ | ||
// vtask will always contains an exception because we know we're not successful and always throws | ||
try | ||
{ | ||
// This does not block because we know that the task already completed | ||
// Using GetAwaiter().GetResult() to automatically unwraps AggregateException inner exception | ||
vtask.GetAwaiter().GetResult(); | ||
} | ||
catch (Exception ex) | ||
{ | ||
FailStage(ex); | ||
return; | ||
} | ||
|
||
throw new InvalidOperationException("Should never reach this code"); | ||
} | ||
else | ||
{ | ||
//We immediately fall into wait case. | ||
//Unlike Task, we don't have a 'status' Enum to switch off easily, | ||
//And Error cases can just live with the small cost of async callback. | ||
async Task ProcessTask() | ||
{ | ||
// Since this Action is used as task continuation, we cannot safely call corresponding | ||
|
@@ -3897,16 +3908,12 @@ async Task ProcessTask() | |
} | ||
} | ||
|
||
#pragma warning disable CS4014 | ||
ProcessTask(); | ||
#pragma warning restore CS4014 | ||
_ = ProcessTask(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discard = no pragma disable needed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handy! |
||
} | ||
} | ||
|
||
public override void OnDownstreamFinish(Exception cause) | ||
{ | ||
_completionCts.Cancel(); | ||
_completionCts.Dispose(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe that's the case. |
||
CompleteStage(); | ||
base.OnDownstreamFinish(cause); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adapted from #6903 report.
Calling
Materializer.Shutdown()
is, as far as I know, going to happen when the actorsystem shuts down.OTOH this test was added in a red->green refactoring, so I know without the changes in
AsyncEnumerable
it fails.