What are the risks in eliminating sync over async?

Question

I'm reviewing changes to a widely used library, which are supposed to be refactorings, and so we want to minimize the risk of introducing any accidental regression. Of course, there are changes from this:

    if (!BlobExists()) 

to this:

    if (!await BlobExists(cancellationToken))

This looks like an impure refactoring - even ignoring the CancellationToken parameter, the return type of BlobExists must have changed to Task<bool>. So, whether the code blocks the thread while the blob existence is checked on some other thread has changed. But, I also know in normal usage the intent of the code is probably the same, and in most cases we are unlikely to depend on the thread-blocking sync-waiting-on-async-result behavior.

That is, at least we are unlikely to know that we intended to depend on the thread-blocking sync-over-async behavior. What are the risk scenarios here, where we might unknowingly actually be depending on that behavior, or in general, unpleasantly surprised to find we've introduced a bug?

Answer 1

For a concrete example of a race condition that could be introduced, consider

using System;
                    
public class Program
{
    public static void Main()
    {
        setupStuff();
        System.Threading.Thread.Sleep(500);
        Console.WriteLine(x); // 7
    }
    
    public static int x = 2;
    
    public static void setupStuff() {
        if (BlobExists()) {
            x = 7;
        }
    }
    
    public static Boolean BlobExists() {
        System.Threading.Thread.Sleep(1000);
        return true;
    }
}

vs

using System;
                    
public class Program
{
    public static void Main()
    {
        setupStuff();
        System.Threading.Thread.Sleep(500);
        Console.WriteLine(x); // 2
    }
    
    public static int x = 2;
    
    public static async void setupStuff() {
        if (await BlobExists()) {
            x = 7;
        }
    }
    
    public static System.Threading.Tasks.Task<Boolean> BlobExists() {
        return System.Threading.Tasks.Task.Run(() => {
                System.Threading.Thread.Sleep(1000);
                return true;
        });

    }
}

Answer 2

Since I want a quick answer to my question, I'm going to start drafting one. I fully expect a better answer will be posted by someone else in the fullness of time...

Let's try to figure it out from first principles by describing more fully the change we are making. Let's also ease the discussion slightly by renaming the Task-returning version of the function BlobExistsAsync(), and calling it 'the async version'.

What do the async version and sync version really do?

1. They both start running on the calling thread, likely, but not necessarily a worker thread. It could be some other thread, like an IO thread, or UI thread. We don't know.
2. At some point, each implementation sends a request, or in reality, they call an API, to talk over the network. They most probably call different APIs under the hood (difference #1). Because they call different APIs, they do some different preparation for this, like in the async case, a continuation function has to be generated (difference #2).
3. The continuation function may or may not actually run, because the API may just return with a result immediately, this is either success, or failure. In this case, both functions probably just return this result back to the calling thread immediately.
4. But what else could happen at this point, when there is no immediate result? Well, it depends on which API we called. In the sync case, the API steals the thread. The current thread keeps running, but it's no longer running our code - its either stuck running the API we called, OR just sleeping waiting for some signal to wake it up again. While it is doing this, it still holds any locks or mutexes that we acquired that are associated to that thread. Also, we have no certainty the API will ever return, it could deadlock, and our thread would certainly never get to run again.
5. But, in the async case, the API yields the thread by returning us a task object, which represents the unfinished work. Eventually, we think, the task will get completed. It's possible to decide to await the task. It's also possible not to, if we don't want to, or we don't want to check the result directly, at least not until later. Either way, we have our thread back, and the task has a continuation function that it can call when it is done. We can continue do more work, while still holding any locks, or mutexes etc. that are associated to this current thread.
6. We can also release those locks, if we have any. We will mainly do this in a race with the calling of the continuation function, when the task completes, so unless we do something extra we can't assume those locks are still held.
7. We also can't assume those locks are NOT still held. (Especially deadlock risk....)
8. The continuation function may get called back on a thread pool worker thread, or an IO thread, or even some other thread. We don't really know, by default, when we are talking about the raw continuation. If we want to be very precise about the kind of context or thread our callback completion runs, there could be extra work involved, like (ick!) using SynchronizationContexts, Posting window message, or using some other dispatch mechanism to offload the work.
9. When we finally use await on the Task, the framework sets up certain things (like SynchronizationContexts or ExecutionContexts) by convention, so that we get called back in the 'least surprising' way possible. Unless we tell it not to, by using .ConfigureAwait(false)...
10. Finally, we get called back by the framework, and our 'continuation' runs, i.e. the rest of the method that we introduced async to.

So, to try to summarize some of the interesting differences that might somehow relate to bugs or changes in behavior of your code:

• your code can be running a different thread, or a different thread from before. This can mean that e.g. it no longer holds a lock acquired via Monitor.TryEnter() or its ilk, that you expected it to (because its owned by a different thread), and a) you probably won't be able to release it since your thread isn't the one associated with it any more. (Luckily the compiler knows not to let you do this the lock statement way.) b) you will read different values from [ThreadStatic] fields than you used to c) your code is now running on an IO thread or UI thread and can block the IO threads instead of jut blocking the worker threads that it used to be running on.

• your code has now introduced wait dependencies between threads (or categories of threads) which may not have existed before. This can contribute to circular wait dependencies, or 'spiral of death' issues.