Potential deadlocks due to .Result calls?

[dracan]

Scanning through the code ready for the codeathon tomorrow, I've spotted a few .Result calls. One example is in SendRequestStatusToGetASmokeAlarm.Send, where it does this: _httpClient.SendAsync(request).Result. There are a few more examples though. This can cause deadlocks and should be avoided. I've lost countless hours in the past due to someone using .Result, and causing random deadlocks.

[bernard-chen]

Are you suggesting that each case be fixed by propagating the async to all callers or did you have a different solution in mind?

[valdisiljuconoks]

Async all the way down would be ideal. But it's bad habit to run sync over async :) One of the workaround could be:

public static class AsyncHelper
{
    private static readonly TaskFactory _myTaskFactory = new TaskFactory(CancellationToken.None,
                                                                         TaskCreationOptions.None,
                                                                         TaskContinuationOptions.None,
                                                                         TaskScheduler.Default);

    public static TResult RunSync<TResult>(Func<Task<TResult>> func)
    {
        return _myTaskFactory.StartNew(func)
                             .Unwrap()
                             .GetAwaiter()
                             .GetResult();
    }

    public static void RunSync(Func<Task> func)
    {
        _myTaskFactory.StartNew(func)
                      .Unwrap()
                      .GetAwaiter()
                      .GetResult();
    }
}

[dracan]

@bernard-chen Yes, if possible, async top to bottom is the ideal. And in most cases nowadays, this is the norm. Obviously, in some cases this isn't possible - but without me going through the code and seeing how it all works, I don't know if it is possible or not in these cases. But if you can't do top to bottom - it's important that the consequences and how synchronisation contexts work are understood - as deadlocks can be really nasty, and very hard to know what's going on. As I say, I've been bitten by this on more than one occasion :(

[bernard-chen]

I agree with async from top to bottom. In those cases where it isn't possible, I think more careful work should be done and maybe those cases should be separated from the more straightforward ones.

[valdisiljuconoks]

@dracan , @bernard-chen I've used AsyncHelper in cases when async is not possible due to framework, platform, library, whatever - and it does its job. Agree that those special cases have to isolated and treated very special - more like an exceptional situation and not normal one.

[stevejgordon]

I agree that this should be fixed. If there are rare places we can't do async all the way, we should record those and decide what the options are in each case.

Therefore I'd suggest the scope of this is to properly await any async calls wherever .Result or .GetAwaiter().GetResult() calls are made.

Good spot @dracan