Remove "Result" property of ValueTask<T>

[ljw1004]

I think we should remove the "Result" property

• I think it's bad to have implicit blocking. If you invoke an async method in a UI app, and try to fetch the .Result of the returned task/valuetask, you’ll get a deadlock.
• ValueTask.Result has different exception behavior from Task.Result: the latter wraps them up in an AggregateException but the former doesn’t.
• There is a niche scenario where .Result is what you want: namely, when you’ve got a synchronous existing codebase and you need to add async into a small corner of it. But I think it’s fine for folks to write “.GetAwaiter().GetResult()” in this case. The extra wordiness of this workaround is commensurate with how carefully you should use it.
• It was a shame to have .Wait() and .Result on the return type of async methods in the first place. We discussed having an ITask instead, a pure promise-like thing which has the advantage of lacking them, but the disadvantage of dealing with an entirely new type was too much.

[dnfclas]

Hi @ljw1004, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

[dnfclas]

@ljw1004, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

[benaadams]

In corefx XmlTextReaderImplAsync would additionally need change too? (1 line)

https://github.com/dotnet/corefx/blob/master/src/System.Xml.ReaderWriter/src/System/Xml/Core/XmlTextReaderImplAsync.cs#L2781

/cc @stephentoub

[benaadams]

Upstream in KestrelHttpServer would need change too? (6 lines)

https://github.com/aspnet/KestrelHttpServer/blob/dev/src/Microsoft.AspNetCore.Server.Kestrel/Http/MessageBody.cs#L62
https://github.com/aspnet/KestrelHttpServer/blob/dev/src/Microsoft.AspNetCore.Server.Kestrel/Http/MessageBody.cs#L85
https://github.com/aspnet/KestrelHttpServer/blob/dev/src/Microsoft.AspNetCore.Server.Kestrel/Http/MessageBody.cs#L186

https://github.com/aspnet/KestrelHttpServer/blob/dev/src/Microsoft.AspNetCore.Server.Kestrel/Http/FrameRequestStream.cs#L72

https://github.com/aspnet/KestrelHttpServer/blob/dev/src/Microsoft.AspNetCore.Server.Kestrel/Filter/LibuvStream.cs#L68
https://github.com/aspnet/KestrelHttpServer/blob/dev/src/Microsoft.AspNetCore.Server.Kestrel/Filter/LibuvStream.cs#L77

[stephentoub]

cc: @terrajobst, @KrzysztofCwalina, @weshaggard

would additionally need change too

Yes. Lucian, you would need to fix all existing uses of this as well. In this PR, please fix all uses in corefx, so that we don't then break when these changes get built into packages and we try to consume those packages. I assume ASP.NET would want you to submit fixes there at the same time so that they don't break when they consume the latest. Please also fix corefxlab, where the Channels project uses ValueTask.

If you invoke an async method in a UI app, and try to fetch the .Result of the returned task/valuetask, you’ll get a deadlock.

The same is true of .GetAwaiter().GetResult(). You can argue that's better because it's not a property and it's more typing, but it's still blocking. Further, blocking can be necessary more often than we'd like.

ValueTask.Result has different exception behavior from Task.Result: the latter wraps them up in an AggregateException but the former doesn’t.

So there can't ever be a new API named "Result" that has different exception-related behaviors from a "Result" we shipped in .NET 4? 😄

I think we should remove the "Result" property

You've not mentioned the other impact here: it's now a bit harder to get the value out of a ValueTask created from a value even if you know there won't be blocking, e.g. if IsCompleted is true, if you created the task from the ctor "new ValueTask(42)", etc. To extract that value, you previously could just say ".Result", which is simple and cheap. If you're in an async method, you can use "await valueTask", but you're not always in an async method when using this type. So now to extract the value, you need to say ".GetAwaiter().GetResult()".

From my perspective, this change does have cost and doesn't improve much: if someone wants to get the value, they're now just going to type more, typing ".GetAwaiter().GetResult()" instead of ".Result", or worse writing an async method that awaits the thing and then calling ".Result" on the returned Task<T>.

But for all my "devil's advocate" pushback in this comment, I actually don't feel strongly about it. If others like the idea of removing it, I'm fine with it.

[benaadams]

Maybe replace with something like?

bool TryGetResult<T>(out T value)
{
    if (_task == null)
    {
        value = _result;
        return true;
    }
    else if (_task.Status == TaskStatus.RanToCompletion)
    {
        value = _task.GetAwaiter().GetResult();
        return true;
    }
    value = default(T);
    return false;
}

As it internalizes dependency on the pattern to avoid blocking; but still allows the fast path for completed things.

[stephentoub]

Maybe replace with something like?

We should separate out removing the existing member from adding a replacement. One needs to be done immediately if it's going to be done, the other can be done if desired later.

That said, I'm not sure this adds much. We're talking about the difference between:

if (vt.IsCompletedSuccessfully)
{
    Use(vt.Result);
}
else
{
    ...
}

vs

TResult result;
if (vt.TryGetResult(out result))
{
    Use(result);
}
else
{
    ...
}

but I don't have any fundamental objections to adding a TryGetResult method at some point if folks found it helpful. It does map to the Try* pattern, so maybe that would be useful for folks that go in search of something like it.

[benaadams]

We're talking about the difference between:

First relies on the user correctly writing non-blocking checks, which is one of the suggested concerns here with .Result; whereas the second means you'd be less likely to do that.

Try* with out is a slightly more annoying pattern though I think dotnet/roslyn#6183 is to address that.

It could be argued it would be a good method to add to Task itself as you may accidentally use _task.IsCompleted rather than _task.Status == TaskStatus.RanToCompletion which is wrong for an exception situation if you are doing something like aspnet/KestrelHttpServer#863

Will raise an suggestion/issue...

[benaadams]

Raised issue https://github.com/dotnet/corefx/issues/8931

[Aaronontheweb]

First relies on the user correctly writing non-blocking checks, which is one of the suggested concerns here with .Result; whereas the second means you'd be less likely to do that.

I trust the users to do the right thing; no need to baby-proof something that's already been in .NET for years. This doesn't add any value IMHO.

[terrajobst]

Personally, I don't see this adding much value. As @stephentoub said, we're blocking regardless. However, .Result is well-known at this point and I think there is a lot of value to have a similar API to Task, hence the naming choice in the first place.

Also, considering we're quite late in Escrow and there are many upstream dependencies outside of CoreFx I'm inclined to say no 👎

@weshaggard? @KrzysztofCwalina?

[ljw1004]

Okay, closing this pull request.