buvinghausen / TaskTupleAwaiter

Async helper library to allow leveraging the new ValueTuple data types in C# 7.0
MIT License
70 stars 10 forks source link

Implement ICriticalNotifyCompletion #5

Closed jnm2 closed 6 years ago

jnm2 commented 6 years ago

tl;dr This is an optimization encouraged for all custom awaiters. Since it makes sense on TaskAwaiter, it makes sense on our syntactic wrapper around TaskAwaiter.

Background from Stephen Toub's excellent https://blogs.msdn.microsoft.com/pfxteam/2012/02/29/whats-new-for-parallelism-in-net-4-5-beta:

The second new interface is ICriticalNotifyCompletion, which inherits from INotifyCompletion. In addition to the OnCompleted method, it also exposes an UnsafeOnCompleted method:

public interface ICriticalNotifyCompletion : INotifyCompletion
{
    [SecurityCritical]
    void UnsafeOnCompleted(Action continuation);
}

For those of you familiar with ThreadPool.UnsafeQueueUserWorkItem, you might be able to guess what is UnsafeOnCompleted. In short, it’s exactly the same as INotifyCompletion.OnCompleted, except that it doesn’t need to flow ExecutionContext.

If an awaiter implements just INotifyCompletion, obviously the compiler will target its OnCompleted when implementing the async method’s state machine. If, however, an awaiter also implements ICriticalNotifyCompletion, the compiler will target its UnsafeOnCompleted method.

For those of you familiar with ExecutionContext, at this point you’re likely asking yourself, “What? How is that safe? Doesn’t ExecutionContext need to flow across await points?” Yes, it does. However, what we discovered with the Async CTP and with the .NET 4.5 Developer Preview is that many folks didn’t realize that their awaiters needed to flow ExecutionContext in order to ensure context flowed across await points (even for those that did, this was a bit of a headache to implement). So, for .NET 4.5 Beta, we’ve modified the async method builders in the Framework (e.g. AsyncTaskMethodBuilder); these are the types targeted by the compilers when building the state machines for async methods. The builders now themselves flow ExecutionContext across await points, taking that responsibility away from the awaiters. That’s why it’s ok for the compilers to target UnsafeOnCompleted, because the builders will handle ExecutionContext flow. That’s also why we needed the interfaces for awaiters, such that the builders could be passed references to awaiters and be able to invoke their {Unsafe}OnCompleted methods polymorphically. Your next question then is likely, “Great, the builder is handling ExecutionContext flow… so why do we need two *OnCompleted methods? Why can’t we just have an OnCompleted that doesn’t flow context?” I’m glad you asked. If you’re building an assembly with AllowPartiallyTrustedCallersAttribute (APTCA) applied to it, you need to ensure that any publicly exposed APIs from your assembly correctly flow ExecutionContext across async points… failure to do so can be a big security hole. As awaiter types will often be implemented in APTCA assemblies, and since OnCompleted could be called directly by a user (even though it’s really meant to be used by the compiler), OnCompleted needs to flow ExecutionContext. But if OnCompleted flows ExecutionContext, and if the builder flows ExecutionContext, now we’ll be flowing ExecutionContext twice. While that’s not a problem functionally, it is an unnecessary and (potentially) non-trivial performance overhead. So, we also have UnsafeOnCompleted, which doesn’t need to flow ExecutionContext, but which is also marked as SecurityCritical, such that partially trusted code can’t call it. If you’re implementing your own awaiter, whenever possible implement both INotifyCompletion and ICriticalNotifyCompletion, flowing ExecutionContext in the former and not flowing it in the latter. The only good reason not to implement both is if you’re implementing an awaiter in a situation where you can’t flow ExecutionContext, e.g. where your awaiter is partially trusted or where you otherwise don’t have the ability to use ExecutionContext, or where the APIs on which your awaiter relies doesn’t give you any option as to whether to flow context or not… in such cases, you can just implement INotifyCompletion.

See also: https://docs.microsoft.com/dotnet/csharp/language-reference/language-specification/expressions#runtime-evaluation-of-await-expressions