Share CompletedTask between Task and AsyncTaskMethodBuilder

[benaadams]

Just the CompletedTask sharing from #24431

/cc @stephentoub @jkotas

[GrabYourPitchforks]

There may be application or framework code that special-cases Task<T> (instead of simply Task) and which might be affected by this. For example, Full Framework MVC observes Task instances returned by action methods, uses reflection to figure out if it's really a Task<T>, pulls out the T value, and operates on it. I don't know if MVC Core has the same behavior.

Since changing the runtime type of Task.CompletedTask could affect those components, we may want to flight this change in an early preview to see if anybody is broken in practice.

[jkotas]

The benefit of this change is minuscule, like maybe 500 bytes saved total in the best case. Given the breaking potential described by @GrabYourPitchforks, I do not think we should take it - even for next version.

[benaadams]

If it was going to break things it should be breaking them now?

All async Tasks are currently Task<VoidTaskResult> and not Task as everything including non-generic async go through the generic AsyncTaskMethodBuilder<TResult> with the non-generic Task using as its type VoidTaskResult.

So you are currently only getting the non-generic CompletedTask if you are manually using it rather than using async and await.

[benaadams]

The alternative would be to go a bit further with #24431 and drop VoidTaskResult altogether; but add some code duplication for the non-generic case.

However I do think anything that can't handle a Task<VoidTaskResult> instead of Task should already and always have been broken by any use of async and await; but maybe I'm wrong /cc @stephentoub

[stephentoub]

I do think anything that can't handle a Task instead of Task should already and always have been broken by any use of async and await; but maybe I'm wrong

It would certainly be possible for someone to depend on Task.CompletedTask returning a concrete Task rather than a Task<VoidTaskResult>; just because many other Task-returning APIs actually return a Task<T> doesn't negate that. However, I agree that it's very unlikely, given that in general code can't depend on a Task not being a Task<T>. I'm surprised to hear that MVC was successful in using reflection in that manner; I'd have expected lots of things to break.

That said, I don't have a strong opinion about whether the savings of one static field and one allocation for the whole process is worthwhile here: I defer to @jkotas' expertise on that.

[benaadams]

That said, I don't have a strong opinion about whether the savings of one static field and one allocation for the whole process is worthwhile here: I defer to @jkotas' expertise on that.

Also means

var task = MethodAsync();
if (ReferenceEquals(task, Task.CompletedTask))
{
    // ...

When used instead of

var task = MethodAsync();
if (task.IsCompletedSucessfully)
{
    // ...

Will work in more instances; umm.... not that I've seen it hanging around in any pre-Core 2.0 legacy code... (when .IsCompletedSucessfully was introduced) https://github.com/aspnet/AspNetCore/blob/15fb5b9e83a99c810ab82716e1f82d4b9378e65a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs#L762-L765 😨

Probably should make a PR there also...

[stephentoub]

Will work in more instances

That hurts me.

[benaadams]

Will work in more instances

That hurts me.

I've been doing everything I could to avoid using it as the real reason for this change...

[benaadams]

dotnet/aspnetcore#11606

[jkotas]

I don't have a strong opinion about whether the savings of one static field and one allocation for the whole process is worthwhile here

I think this is a good change to take once we open master for .NET 5 (~3 weeks).

Task has a lot of internal fields, so one more does not make things look that much worse.

[stephentoub]

Replaced by #27437