Remove CancelableEnumerator #10099
Remove CancelableEnumerator #10099
Conversation
mikaelm12
requested review from
BrennanConroy,
analogrelay and
halter73
as code owners
| try | ||
| { | ||
| await foreach (var streamItem in enumerable) | ||
| var enumerator = enumerable.GetAsyncEnumerator(streamCts.Token); |
There was a problem hiding this comment.
Should the enumerator be disposed?
There was a problem hiding this comment.
Leave the await foreach and call .WithCancellation instead:
await foreach (var streamItem in enumerable.WithCancellation(streamCts.Token))
{
}
|
We should make |
| if (cancellationToken.CanBeCanceled) | ||
| { | ||
| var registration = cancellationToken.Register((ctsState) => | ||
| { |
There was a problem hiding this comment.
registration not referenced. Actually is the private class still needed?
There was a problem hiding this comment.
The class is still needed because we need to be able to cancel the inner stream operation from the token passed into GetASyncEnumerator but you bring up a good point about the registration. It should be disposed
|
This comment was made automatically. If there is a problem contact [email protected]. I've triaged the above build. I've created/commented on the following issue(s) |
|
So in another PR we should work to remove more from the |
ghost
deleted the
| { | ||
| // Assume that this will be iterated through with await foreach which always passes a default token. | ||
| // Instead use the token from the ctor. | ||
| Debug.Assert(cancellationToken == default); |
There was a problem hiding this comment.
@mikaelm12 Why remove this Debug.Assert if we're still not using the cancellationToken being passed in? Don't we now need to link the token sources or assert the cancellationToken is not cancellable? Otherwise we're just ignoring it...
This reverts commit d37b2ca.
Fixes: #7960
Context: #6791 (comment)