Obsolete Task<T>.Result, Introduce Task<T>.TryGetResult #8931

Closed
benaadams opened this Issue May 27, 2016 · 26 comments

Comments

Projects
None yet
@benaadams
Collaborator

benaadams commented May 27, 2016

Related #8902 "Remove "Result" property of ValueTask"

  • Task<T>.Result has can have implicit blocking. If you invoke an async method in a UI app, and try to fetch the .Result of the returned task before it is completed, you’ll get a deadlock.
  • .GetAwaiter().GetResult() will force a blocking value return if required
  • An expected test to see if the result is ready without exceptions may be task.IsCompleted, however it should really be task.Status == TaskStatus.RanToCompletion

An alternative bool TryGetResult<T>(out T value) would be a safer option increasing the chance of writing correct code.

It would return true if the task has RanToCompletion with the result in the out param.

It would return false if the task hadn't completed (or was cancelled/faulted, maybe throw exception then)

@svick

This comment has been minimized.

Show comment
Hide comment
@svick

svick May 28, 2016

Contributor

I think that task.Result is still useful for .Net 4-style parallel computations. You can use async-await for those too, but it shouldn't be necessary.

I don't think that task.GetAwaiter().GetResult() is an acceptable alternative, but task.Wait(); task.TryGetResult(out var result); might be.

Contributor

svick commented May 28, 2016

I think that task.Result is still useful for .Net 4-style parallel computations. You can use async-await for those too, but it shouldn't be necessary.

I don't think that task.GetAwaiter().GetResult() is an acceptable alternative, but task.Wait(); task.TryGetResult(out var result); might be.

@GSPP

This comment has been minimized.

Show comment
Hide comment
@GSPP

GSPP May 28, 2016

Wouldn't it be a good idea to keep Task and ValueTask aligned where possible?

.GetAwaiter().GetResult() has different exception propagation behavior than Result and Wait have. In particular the fact that it tends to throw away exceptions makes this option not entirely harmless.

Maybe TryGetResult should be added to both types regardless.

GSPP commented May 28, 2016

Wouldn't it be a good idea to keep Task and ValueTask aligned where possible?

.GetAwaiter().GetResult() has different exception propagation behavior than Result and Wait have. In particular the fact that it tends to throw away exceptions makes this option not entirely harmless.

Maybe TryGetResult should be added to both types regardless.

@nguerrera

This comment has been minimized.

Show comment
Hide comment
@nguerrera

nguerrera May 29, 2016

Member

I think something that indicates that it will only retrieve the result if the task completed successfully should be in the name. Otherwise, I imagine the only difference between Result and TryGetResult to be that the former can raise an exception, not that the latter will never block.

Perhaps, TryGetSuccessfulResult?

Member

nguerrera commented May 29, 2016

I think something that indicates that it will only retrieve the result if the task completed successfully should be in the name. Otherwise, I imagine the only difference between Result and TryGetResult to be that the former can raise an exception, not that the latter will never block.

Perhaps, TryGetSuccessfulResult?

@GSPP

This comment has been minimized.

Show comment
Hide comment
@GSPP

GSPP May 29, 2016

Or a boolean argument bool allowThrow. That way the caller is forced to pick what he wants.

GSPP commented May 29, 2016

Or a boolean argument bool allowThrow. That way the caller is forced to pick what he wants.

@jnm2

This comment has been minimized.

Show comment
Hide comment
@jnm2

jnm2 Jun 1, 2016

Collaborator

Given the strong warnings against using .Result and .Wait() by the ASP.NET team and Stephen Cleary (1), (2) and others because of deadlocks, wrapped exceptions, and plain inefficiency, I consider them a strong code smell. They are a pitfall for so many people. They're obnoxious.

Collaborator

jnm2 commented Jun 1, 2016

Given the strong warnings against using .Result and .Wait() by the ASP.NET team and Stephen Cleary (1), (2) and others because of deadlocks, wrapped exceptions, and plain inefficiency, I consider them a strong code smell. They are a pitfall for so many people. They're obnoxious.

@Mike-EEE

This comment has been minimized.

Show comment
Hide comment
@Mike-EEE

Mike-EEE Sep 7, 2016

Nice idea, it has got my upvote. I have added it to my growing list of issues and discontent with TPL. Although this looks to fix/address a few of them. 😎

Mike-EEE commented Sep 7, 2016

Nice idea, it has got my upvote. I have added it to my growing list of issues and discontent with TPL. Although this looks to fix/address a few of them. 😎

@stephentoub stephentoub removed their assignment Sep 30, 2016

@karelz

This comment has been minimized.

Show comment
Hide comment
@karelz

karelz Oct 13, 2016

Member

Looks useful. Can someone put together formal API proposal please?

Member

karelz commented Oct 13, 2016

Looks useful. Can someone put together formal API proposal please?

@terrajobst

This comment has been minimized.

Show comment
Hide comment
@terrajobst

terrajobst Nov 7, 2016

Member

Hey @stephentoub, what are your thoughts on this?

We can obviously not remove the blocking APIs but it might be worthwhile considering doing something like obsoleting them or providing an analyzer. My gut feel is that obsoletion is the wrong thing because it's too widely used, and there are many cases where the usage is safe because the caller knows that the task has already completed (e.g. after awaiting multiple tasks).

We could also consider adding a new API that will only work when the task is completed and throw when it's not completed yet. This API should probably have a TryXxx counterpart.

Thoughts?

Member

terrajobst commented Nov 7, 2016

Hey @stephentoub, what are your thoughts on this?

We can obviously not remove the blocking APIs but it might be worthwhile considering doing something like obsoleting them or providing an analyzer. My gut feel is that obsoletion is the wrong thing because it's too widely used, and there are many cases where the usage is safe because the caller knows that the task has already completed (e.g. after awaiting multiple tasks).

We could also consider adding a new API that will only work when the task is completed and throw when it's not completed yet. This API should probably have a TryXxx counterpart.

Thoughts?

@stephentoub

This comment has been minimized.

Show comment
Hide comment
@stephentoub

stephentoub Nov 9, 2016

Member

Hey @stephentoub, what are your thoughts on this?

We should not obsolete them; as highlighted, there are valid uses. I'd be fine with an analyzer that warned about blocking behaviors in general, not just limited to Task, but rather an analyzer that, for example, warned any time you were using a synchronous/blocking API when an asynchronous alternative existed. I do expect that such an analyzer would have a relatively high noise to signal ratio, though.

We could also consider adding a new API that will only work when the task is completed and throw when it's not completed yet. This API should probably have a TryXxx counterpart.

Essentially what's being proposed is:

public static bool TryGetResult<T>(this Task<T> task, out T result)
{
    if (task.Status == TaskStatus.RanToCompletion)
    {
        result = task.Result;
        return true;
    }
    result = default(T);
    return false;
}

? Eh. I don't have any real problem with it, but I'm not sure how useful it is. In my experience, if you're trying to get a task's result, you typically either want the result regardless of whether the task has already completed (e.g. await t;, t.Result, or t.GetAwaiter().GetResult()), or you want to know the status of the task and be able to distinguish the various cases of it not having a result (not yet completed vs completed but due to error/cancellation)... for that you're going to check either its Status or its various Is properties. In neither of those predominant use cases would TryGetResult be valuable.

My $.02.

Member

stephentoub commented Nov 9, 2016

Hey @stephentoub, what are your thoughts on this?

We should not obsolete them; as highlighted, there are valid uses. I'd be fine with an analyzer that warned about blocking behaviors in general, not just limited to Task, but rather an analyzer that, for example, warned any time you were using a synchronous/blocking API when an asynchronous alternative existed. I do expect that such an analyzer would have a relatively high noise to signal ratio, though.

We could also consider adding a new API that will only work when the task is completed and throw when it's not completed yet. This API should probably have a TryXxx counterpart.

Essentially what's being proposed is:

public static bool TryGetResult<T>(this Task<T> task, out T result)
{
    if (task.Status == TaskStatus.RanToCompletion)
    {
        result = task.Result;
        return true;
    }
    result = default(T);
    return false;
}

? Eh. I don't have any real problem with it, but I'm not sure how useful it is. In my experience, if you're trying to get a task's result, you typically either want the result regardless of whether the task has already completed (e.g. await t;, t.Result, or t.GetAwaiter().GetResult()), or you want to know the status of the task and be able to distinguish the various cases of it not having a result (not yet completed vs completed but due to error/cancellation)... for that you're going to check either its Status or its various Is properties. In neither of those predominant use cases would TryGetResult be valuable.

My $.02.

@benaadams

This comment has been minimized.

Show comment
Hide comment
@benaadams

benaadams Nov 9, 2016

Collaborator

@terrajobst I think @stephentoub makes a lot of sense. Closing.

Collaborator

benaadams commented Nov 9, 2016

@terrajobst I think @stephentoub makes a lot of sense. Closing.

@benaadams benaadams closed this Nov 9, 2016

@karelz karelz modified the milestones: Future, 1.2.0 Dec 3, 2016

@slang25

This comment has been minimized.

Show comment
Hide comment
@slang25

slang25 Apr 13, 2017

Another option would be to set the SynchronizationContext to null and then return it. In my experience these issues come deep into the call stack where people are well away from places that require/assume a sync context, shouldn't we be steering people towards a blocking usage that doesn't deadlock?

Something like (naming is hard!):

public static T GetResultSafely<T>(this Task<T> task)
{
    var capturedContext = SynchonizationContext.Current;
    try {
        SynchonizationContext.SetSynchronizationContext(null);
        return task.GetAwaiter().GetResult();
    }
    finally {
        SynchonizationContext.SetSynchronizationContext(capturedContext);
    }
}

The xml doc comments could make it clear you will not have access the the sync context within here.
This is what I am recommending to my colleagues as we use 3rd party libraries we cannot be sure use await responsibly.

I'm not sure why this wouldn't be the default. Would love to hear your thoughts @stephentoub

slang25 commented Apr 13, 2017

Another option would be to set the SynchronizationContext to null and then return it. In my experience these issues come deep into the call stack where people are well away from places that require/assume a sync context, shouldn't we be steering people towards a blocking usage that doesn't deadlock?

Something like (naming is hard!):

public static T GetResultSafely<T>(this Task<T> task)
{
    var capturedContext = SynchonizationContext.Current;
    try {
        SynchonizationContext.SetSynchronizationContext(null);
        return task.GetAwaiter().GetResult();
    }
    finally {
        SynchonizationContext.SetSynchronizationContext(capturedContext);
    }
}

The xml doc comments could make it clear you will not have access the the sync context within here.
This is what I am recommending to my colleagues as we use 3rd party libraries we cannot be sure use await responsibly.

I'm not sure why this wouldn't be the default. Would love to hear your thoughts @stephentoub

@stephentoub

This comment has been minimized.

Show comment
Hide comment
@stephentoub

stephentoub Apr 13, 2017

Member

Would love to hear your thoughts @stephentoub

I don't see how the proposed function helps. By the time you're blocking waiting on the task, you've already invoked the asynchronous operation that in turn may have observed the current synchronization context and captured it. This helper would instead need to take a delegate that could be invoked while the SynchronizationContext was null, rather than taking the task to wait on.

Member

stephentoub commented Apr 13, 2017

Would love to hear your thoughts @stephentoub

I don't see how the proposed function helps. By the time you're blocking waiting on the task, you've already invoked the asynchronous operation that in turn may have observed the current synchronization context and captured it. This helper would instead need to take a delegate that could be invoked while the SynchronizationContext was null, rather than taking the task to wait on.

@slang25

This comment has been minimized.

Show comment
Hide comment
@slang25

slang25 Apr 13, 2017

Very good point @stephentoub.

public static T ExecuteSynchronously<T>(Func<Task<T>> taskFunc)
{
    var capturedContext = SynchronizationContext.Current;
    try {
        SynchronizationContext.SetSynchronizationContext(null);
        return taskFunc.Invoke().GetAwaiter().GetResult();
    }
    finally {
        SynchronizationContext.SetSynchronizationContext(capturedContext);
    }
}

Would this be worth considering.

slang25 commented Apr 13, 2017

Very good point @stephentoub.

public static T ExecuteSynchronously<T>(Func<Task<T>> taskFunc)
{
    var capturedContext = SynchronizationContext.Current;
    try {
        SynchronizationContext.SetSynchronizationContext(null);
        return taskFunc.Invoke().GetAwaiter().GetResult();
    }
    finally {
        SynchronizationContext.SetSynchronizationContext(capturedContext);
    }
}

Would this be worth considering.

@stephentoub

This comment has been minimized.

Show comment
Hide comment
@stephentoub

stephentoub Apr 13, 2017

Member

Would this be worth considering

I don't think this is a good thing to add; I don't want to encourage more synchronous blocking on tasks by adding yet another way to do it.

If we were interested in something along those lines (I'm not saying we should), I would rather see something like:

public class SynchronizationContext
{
    public static TResult RunWithCurrent<TState, TResult>(SynchronizationContext target, Func<TState, TResult> function, TState state)
    {
        SynchronizationContext old = SynchronizationContext.Current;
        try { return function(state); }
        finally { SynchronizationContext.SetSynchronizationContext(old); }
    }
}

If you wanted to use that for your purposes, you could, e.g.

SynchronizationContext.RunWithCurrent(null, _ => SomeMethodAsync(), null).GetAwaiter().GetResult();

but it could also be used for non-Task-returning methods.

Member

stephentoub commented Apr 13, 2017

Would this be worth considering

I don't think this is a good thing to add; I don't want to encourage more synchronous blocking on tasks by adding yet another way to do it.

If we were interested in something along those lines (I'm not saying we should), I would rather see something like:

public class SynchronizationContext
{
    public static TResult RunWithCurrent<TState, TResult>(SynchronizationContext target, Func<TState, TResult> function, TState state)
    {
        SynchronizationContext old = SynchronizationContext.Current;
        try { return function(state); }
        finally { SynchronizationContext.SetSynchronizationContext(old); }
    }
}

If you wanted to use that for your purposes, you could, e.g.

SynchronizationContext.RunWithCurrent(null, _ => SomeMethodAsync(), null).GetAwaiter().GetResult();

but it could also be used for non-Task-returning methods.

@slang25

This comment has been minimized.

Show comment
Hide comment
@slang25

slang25 Apr 14, 2017

@stephentoub I assume you are missing a usage of target in there like:

public class SynchronizationContext
{
    public static TResult RunWithCurrent<TState, TResult>(SynchronizationContext target, Func<TState, TResult> function, TState state)
    {
        SynchronizationContext old = SynchronizationContext.Current;
        try {
            SynchronizationContext.SetSynchronizationContext(target);
            return function(state);
        }
        finally { SynchronizationContext.SetSynchronizationContext(old); }
    }
}

slang25 commented Apr 14, 2017

@stephentoub I assume you are missing a usage of target in there like:

public class SynchronizationContext
{
    public static TResult RunWithCurrent<TState, TResult>(SynchronizationContext target, Func<TState, TResult> function, TState state)
    {
        SynchronizationContext old = SynchronizationContext.Current;
        try {
            SynchronizationContext.SetSynchronizationContext(target);
            return function(state);
        }
        finally { SynchronizationContext.SetSynchronizationContext(old); }
    }
}
@stephentoub

This comment has been minimized.

Show comment
Hide comment
@stephentoub

stephentoub Apr 14, 2017

Member

Yup, was typing too quickly. Thanks.

Member

stephentoub commented Apr 14, 2017

Yup, was typing too quickly. Thanks.

@Drawaes

This comment has been minimized.

Show comment
Hide comment
@Drawaes

Drawaes Oct 17, 2017

Collaborator

aspnet/KestrelHttpServer#2104 (comment)

People are constantly caught out by this.

Collaborator

Drawaes commented Oct 17, 2017

aspnet/KestrelHttpServer#2104 (comment)

People are constantly caught out by this.

@Drawaes

This comment has been minimized.

Show comment
Hide comment
@Drawaes

Drawaes Oct 17, 2017

Collaborator

I am less concerned about outright deadlocks and sync contexts, but instead the threadpool starvation is the one that normally gets through testing and kills apps in prod under load

Collaborator

Drawaes commented Oct 17, 2017

I am less concerned about outright deadlocks and sync contexts, but instead the threadpool starvation is the one that normally gets through testing and kills apps in prod under load

@stephentoub

This comment has been minimized.

Show comment
Hide comment
@stephentoub

stephentoub Oct 17, 2017

Member

My answer here hasn't changed from before:
#8931 (comment)

But if the goal is to simply detect when blocking waits are being used, there are ways to do that, e.g. there's already an EventSource event for it:

using System;
using System.Diagnostics.Tracing;
using System.Threading;
using System.Threading.Tasks;

class Program
{
    static void Main()
    {
        TaskBlockingListener.Start();

        Task.Run(() => Thread.Sleep(5000)).Wait();
    }
}

public sealed class TaskBlockingListener : EventListener
{
    private static readonly Guid s_tplGuid = new Guid("2e5dba47-a3d2-4d16-8ee0-6671ffdcd7b5");
    private static readonly Lazy<TaskBlockingListener> s_listener = new Lazy<TaskBlockingListener>(() => new TaskBlockingListener());

    public static void Start() { object ignored = s_listener.Value; }

    private TaskBlockingListener() { }

    protected override void OnEventSourceCreated(EventSource eventSource)
    {
        if (eventSource.Guid == s_tplGuid)
        {
            EnableEvents(eventSource, EventLevel.Informational, (EventKeywords)3); // 3 == Task|TaskTransfer
        }
    }

    protected override void OnEventWritten(EventWrittenEventArgs eventData)
    {
        if (eventData.EventId == 10 && // TASKWAITBEGIN_ID
            eventData.Payload != null &&
            eventData.Payload.Count > 3 &&
            eventData.Payload[3] is int value && // Behavior
            value == 1) // TaskWaitBehavior.Synchronous
        {
            Console.WriteLine("Blocking on an incomplete task." + Environment.NewLine + Environment.StackTrace);
        }
    }
}

cc: @davidfowl

Member

stephentoub commented Oct 17, 2017

My answer here hasn't changed from before:
#8931 (comment)

But if the goal is to simply detect when blocking waits are being used, there are ways to do that, e.g. there's already an EventSource event for it:

using System;
using System.Diagnostics.Tracing;
using System.Threading;
using System.Threading.Tasks;

class Program
{
    static void Main()
    {
        TaskBlockingListener.Start();

        Task.Run(() => Thread.Sleep(5000)).Wait();
    }
}

public sealed class TaskBlockingListener : EventListener
{
    private static readonly Guid s_tplGuid = new Guid("2e5dba47-a3d2-4d16-8ee0-6671ffdcd7b5");
    private static readonly Lazy<TaskBlockingListener> s_listener = new Lazy<TaskBlockingListener>(() => new TaskBlockingListener());

    public static void Start() { object ignored = s_listener.Value; }

    private TaskBlockingListener() { }

    protected override void OnEventSourceCreated(EventSource eventSource)
    {
        if (eventSource.Guid == s_tplGuid)
        {
            EnableEvents(eventSource, EventLevel.Informational, (EventKeywords)3); // 3 == Task|TaskTransfer
        }
    }

    protected override void OnEventWritten(EventWrittenEventArgs eventData)
    {
        if (eventData.EventId == 10 && // TASKWAITBEGIN_ID
            eventData.Payload != null &&
            eventData.Payload.Count > 3 &&
            eventData.Payload[3] is int value && // Behavior
            value == 1) // TaskWaitBehavior.Synchronous
        {
            Console.WriteLine("Blocking on an incomplete task." + Environment.NewLine + Environment.StackTrace);
        }
    }
}

cc: @davidfowl

@davidfowl

This comment has been minimized.

Show comment
Hide comment
@davidfowl

davidfowl Oct 17, 2017

Contributor

Agreed. We can’t remove the API and it’s still useful in a variety of cases. What we can do is use techniques like above to detect common error conditions. Maybe this could be on if a flag is set (detect blocking tasks). It would more effectively help find blocking code all over existing code bases.

Contributor

davidfowl commented Oct 17, 2017

Agreed. We can’t remove the API and it’s still useful in a variety of cases. What we can do is use techniques like above to detect common error conditions. Maybe this could be on if a flag is set (detect blocking tasks). It would more effectively help find blocking code all over existing code bases.

@benaadams

This comment has been minimized.

Show comment
Hide comment
@benaadams

benaadams Oct 17, 2017

Collaborator

if the goal is to simply detect when blocking waits are being used, there are ways to do that,

Is a very nice diagnostic sample; is easier than trying to get people to manually spot in existing code base or wait for the next jam to get a process dump.

I'd be fine with an analyzer that warned about blocking behaviors in general, not just limited to Task, but rather an analyzer that, for example, warned any time you were using a synchronous/blocking API when an asynchronous alternative existed. I do expect that such an analyzer would have a relatively high noise to signal ratio, though.

Which repo should analyser issues get raised?

Collaborator

benaadams commented Oct 17, 2017

if the goal is to simply detect when blocking waits are being used, there are ways to do that,

Is a very nice diagnostic sample; is easier than trying to get people to manually spot in existing code base or wait for the next jam to get a process dump.

I'd be fine with an analyzer that warned about blocking behaviors in general, not just limited to Task, but rather an analyzer that, for example, warned any time you were using a synchronous/blocking API when an asynchronous alternative existed. I do expect that such an analyzer would have a relatively high noise to signal ratio, though.

Which repo should analyser issues get raised?

@Drawaes

This comment has been minimized.

Show comment
Hide comment
@Drawaes

Drawaes Oct 17, 2017

Collaborator

Maybe a listener can be added to ASP.NET by default in debug mode? It could be switched off with a launch setting.

This would be similar to how app insights runs today when you run the default asp.net core project. This would catch where the error often happens and allow for it to be turned off.

Collaborator

Drawaes commented Oct 17, 2017

Maybe a listener can be added to ASP.NET by default in debug mode? It could be switched off with a launch setting.

This would be similar to how app insights runs today when you run the default asp.net core project. This would catch where the error often happens and allow for it to be turned off.

@ayende

This comment has been minimized.

Show comment
Hide comment
@ayende

ayende Nov 8, 2017

Contributor

@stephentoub Is there a source with all these values documents?
And will this work in a cross platform manner?

Contributor

ayende commented Nov 8, 2017

@stephentoub Is there a source with all these values documents?
And will this work in a cross platform manner?

@stephentoub

This comment has been minimized.

Show comment
Hide comment
@stephentoub

stephentoub Nov 9, 2017

Member

Is there a source with all of these values documented?

I don't know if there's any documentation other than the source:
https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/Threading/Tasks/TPLETWProvider.cs

And will this work in a cross platform manner?

Yes

Member

stephentoub commented Nov 9, 2017

Is there a source with all of these values documented?

I don't know if there's any documentation other than the source:
https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/Threading/Tasks/TPLETWProvider.cs

And will this work in a cross platform manner?

Yes

@demosglok

This comment has been minimized.

Show comment
Hide comment
@demosglok

demosglok Nov 27, 2017

why this magic guid?
Guid("2e5dba47-a3d2-4d16-8ee0-6671ffdcd7b5");

why this magic guid?
Guid("2e5dba47-a3d2-4d16-8ee0-6671ffdcd7b5");

@stephentoub

This comment has been minimized.

Show comment
Hide comment
@stephentoub

stephentoub Nov 27, 2017

Member

why this magic guid?

Every ETW provider chooses a unique ID in the form of a guid that every consumer uses to enable/listen to it.

Member

stephentoub commented Nov 27, 2017

why this magic guid?

Every ETW provider chooses a unique ID in the form of a guid that every consumer uses to enable/listen to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment