-
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
cleaned up IAsyncEnumerable
Source to use local functions
#6045
cleaned up IAsyncEnumerable
Source to use local functions
#6045
Conversation
Better to use `await` around `ValueTask` rather than converting to `Task` and doing `ContinueWith`
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.
Looks like the code can be cleaned up further.
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.
@Aaronontheweb Need you to review the changes I've made
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.
LGTM
{ | ||
_enumerator = enumerator; | ||
_completionCts = new CancellationTokenSource(); | ||
_enumerator = enumerable.GetAsyncEnumerator(_completionCts.Token); |
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.
Interesting question, should this be in prestart vs the constructor?
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.
The only thing that would throw, I guess, would be if there was something wrong with the IAsyncEnumerable
(i.e. null
)?
In that case I guess it'd be better to have this run in PreStart
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.
That's a good point, I'll make the appropriate changes
vtask.GetAwaiter().OnCompleted(_onComplete); | ||
} | ||
_completionCts.Cancel(); | ||
_completionCts.Dispose(); |
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.
Do we have guarantees that cancelling will dispose the enumerator if needed?
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.
Oh, you're totally correct, I forgot that we need to dispose the enumerator... I guess the previous code is still needed here.
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.
Here's a big question, should we defer the base.OnDownstreamFinish()
call and do this asynchronously or just block and wait until _enumerator.DisposeAsync
is complete?
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.
public override void OnDownstreamFinish(Exception cause)
{
_completionCts.Cancel();
_completionCts.Dispose();
_enumerator.DisposeAsync().AsTask().ContinueWith(t => _baseOnDownstreamFinishCallback(cause));
}
private void BaseOnDownstreamFinishCallback(Exception cause)
{
CompleteStage();
base.OnDownstreamFinish(cause);
}
or
public override void OnDownstreamFinish(Exception cause)
{
_completionCts.Cancel();
_completionCts.Dispose();
CompleteStage();
_enumerator.DisposeAsync().GetAwaiter().GetResult();
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.
I will add the try...catch later, just wanted to get the point across first
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.
The only problem that I can see here is a custom IAsyncEnumerator
that deadlocked/froze when disposed
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.
I think we should go with the latter - I doubt there will be IAsyncEnumerator.DisposeAsync
implementations that can block for prolonged periods of time.
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.
Done
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.
LGTM
Changes
Better to use
await
aroundValueTask
rather than converting toTask
and doingContinueWith
Cleanup from #6044
Checklist
For significant changes, please ensure that the following have been completed (delete if not relevant):