-
Notifications
You must be signed in to change notification settings - Fork 4.9k
[WIP] Update System.Threading.Channels for .NET Core 3.0 #32904
Conversation
| // Queue the continuation. We always queue here, even if !RunContinuationsAsynchronously, in order | ||
| // to avoid stack diving; this path happens in the rare race when we're setting up to await and the | ||
| // object is completed after the awaiter.IsCompleted but before the awaiter.OnCompleted. | ||
| if (_schedulingContext == null) |
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.
if (_schedulingContext == null) [](start = 16, length = 31)
Is it possible at all to have _schedulingContext == null and sc != null? and if yes, shouldn't we still need to do sc.Post at that 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.
See the code above that assigns _schedulingContext: it's only set to sc if the sc is one we want to respect.
That said, looking at it again there's an existing very-corner-case bug here, pre-existing this change... it's possible sc could be non-null, ts could be non-null, and we want to use the ts, but we'll end up using the sc instead. I'll fix that separately. We should be setting sc to null when we go to check for a task scheduler.
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.
also it is worth adding some assert here. I mean ensuring the the states of sc with _schedulingContext. just in case in the future anyone update the sc initialization code.
In reply to: 226717506 [](ancestors = 226717506)
| while (TryRead(out T item)) | ||
| { | ||
| yield return item; | ||
| cancellationToken.ThrowIfCancellationRequested(); |
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.
cancellationToken.ThrowIfCancellationRequested(); [](start = 20, length = 49)
Is it possible we get into the state that TryRead(out T item) succeed and then we do yield return item; but after this point the the cancellation happen. at that time I don't think we need to throw as next TryRead(out T item) could return false and we exit the loop.
in other word, the channel becomes empty and then the cancellation happen.
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 channel becomes empty and then the cancellation happen.
That could be the case no matter where we check.
- If we check before we enter the outer loop, then we won't be checking per item.
- If we check before we enter the inner loop, someone else could have already taken out the item that caused WaitForReadAsync to return true, and then the channel could be marked as completed and TryRead will return false.
- If we check after the TryRead but before the yield we might consume an item and then throw, losing data.
- If we check after the yield, then we're at least checking for each item, but may throw on cancellation even if the channel is complete. We could of course try to check for that, too, e.g.
if (token.IsCancellationRequested && !Completed) cancellationToken.ThrowIfCancellationRequested();, but there's still a race condition there.
|
Do we need to add any tests here? |
Yes. This is WIP because lots of stuff still needs to be done, first and foremost getting all of its dependencies in place, and an updated compiler that supports async iterators (though I may rewrite the async enumerable to avoid the dependency on the compiler for now, and then simplify when we get a new compiler). |
- Avoid ThreadPool-related allocations via IThreadPoolWorkItem. We already had a fairly low allocation profile on most channels, thanks to an IValueTaskSource implementation. This extends that implementation with an IThreadPoolWorkItem implementation so that when we do need to queue to the pool (e.g. to support RunContinuationsAsynchronously, on writes on the bounded queue, etc.), we can do so without incurring additional allocation. - Avoid ExecutionContext costs with CancellationToken.UnsafeRegister. Minor savings when a cancelable token is provided; we don't need to flow context as all we're doing is completing another object.
4fa4ba8 to
0d0758a
Compare
Gated on the API review from https://github.com/dotnet/corefx/issues/32742.
Depends on #32859.
Depends on #32743.
await foreach.Fixes https://github.com/dotnet/corefx/issues/32742
cc: @tarekgh
(This is still a WIP, due to all of the dependencies that haven't yet come into master yet, but I wanted to put it up now as a reference point.)