Is it safe to nest async code inside a lock if it is nested in an additional stack frame?-CodePudding

The question may sound a bit abstract, here's an example of a non async piece of code that does some lazy loading and ensures that the lazy loading is only done once by the first thread:

public class LazyExample
{
    private object ExpensiveResource = null;

    private object ExpensiveResourceSyncRoot = new object();

    public object GetExpensiveResource()
    {
        if (ExpensiveResource != null) // Checkpoint 1
            return ExpensiveResource;

        lock (ExpensiveResourceSyncRoot) // initially there will be a queue of threads waiting here that have already passed Checkpoint 1
        {
            if (ExpensiveResource != null) // prevent re-retrieval by all subsequent threads that passed Checkpoint 1
                return ExpensiveResource;

            // create the expensive resource but do not assign yet...
            object expensiveResource = new object();

            // initialize the expensive resource, for example:
            // - Call an API...
            // - Do some Database lookups...
            Thread.Sleep(1); 

            // finally as last step before releasing the lock, assign the fully initialized expensive object
            ExpensiveResource = expensiveResource;
        }

        return ExpensiveResource;
    }
}

In our lib, the async virus has started to infest many calls. Since awaiting is not allowed directly inside a lock we now wrap the async stuff in a new method like so:

public class LazyExample
{
    private object ExpensiveResource = null;

    private object ExpensiveResourceSyncRoot = new object();

    public async Task<object> GetExpensiveResourceAsync()
    {
        if (ExpensiveResource != null) // Checkpoint 1
            return ExpensiveResource;

        lock (ExpensiveResourceSyncRoot) // initially there will be a queue of threads waiting here that have already passed Checkpoint 1
        {
            if (ExpensiveResource != null) // prevent re-retrieval by all subsequent threads that passed Checkpoint 1
                return ExpensiveResource;
            
            // assign the fully initialized expensive object
            ExpensiveResource = CreateAndInitializeExpensiveResourceAsync().Result;
        }

        return ExpensiveResource;
    }

    private async Task<object> CreateAndInitializeExpensiveResourceAsync()
    {
        object expensiveResource = new object();

        // initialize the expensive resource, this time async:
        await Task.Delay(1);

        return expensiveResource;
    }
}

This however feels like putting a zip-tie around a safety latch and defeating the cause.

In seemingly random cases we need to wrap a call in order to prevent deadlocks like so:

ExpensiveResource = Task.Run(CreateAndInitializeExpensiveResourceAsync).Result;

This will force the method to run in a different thread and cause the current thread to go idle until it joins (extra overhead for no good reason as far as I can tell).

So my question is: is it safe to offload async stuff to a separate method (a new stack frame if you will) inside a lock or are we indeed defeating a safety latch?

CodePudding user response：

Synchronously waiting for your async operation to finish, putting a lock around that, and then punting that off to the ThreadPool to make it async again is a solution, but it's just about the worst option out there.

First, why not use an async lock? SemaphoreSlim is the classic:

public class LazyExample
{
    private object ExpensiveResource = null;

    private readonly SemaphoreSlim ExpensiveResourceSyncRoot = new(1, 1);

    public async Task<object> GetExpensiveResourceAsync()
    {
        if (ExpensiveResource != null) // Checkpoint 1
            return ExpensiveResource;

        await ExpensiveResourceSyncRoot.WaitAsync();
        try
        {
            if (ExpensiveResource != null) // prevent re-retrieval by all subsequent threads that passed Checkpoint 1
                return ExpensiveResource;

            // finally as last step before releasing the lock, assign the fully initialized expensive object
            ExpensiveResource = await CreateAndInitializeExpensiveResourceAsync();
        }
        finally
        {
            ExpensiveResourceSyncRoot.Release();
        }

        return ExpensiveResource;
    }
}

Secondly, there's a much better solution to this problem of lazy async initialisation. Instead of caching ExpensiveResource, cache a Task which represents the initialisation of ExpensiveResource. When a consumer awaits that Task, either they'll find that it's already complete, in which case the resource is available immediately, or they'll have to wait, in which case the resource is still being created, and by awaiting the Task they'll be notified when it is available.

public class LazyExample
{
    private Task<object> ExpensiveResource = null;

    private readonly object ExpensiveResourceSyncRoot = new();

    // Note this is not async, and does not contain awaits
    public Task<object> GetExpensiveResourceAsync()
    {
        if (ExpensiveResource != null)
            return ExpensiveResource;

        lock (ExpensiveResourceSyncRoot)
        {
            if (ExpensiveResource != null)
                return ExpensiveResource;

            // Note that we don't await this.
            // ExpensiveResource holds the Task which represents the creation of the resource
            ExpensiveResource = CreateAndInitializeExpensiveResourceAsync();
        }

        return ExpensiveResource;
    }
}

We can further simplify with a Lazy<T>:

public class LazyExample
{
    private readonly Lazy<Task<object>> ExpensiveResource;

    public LazyExample()
    {
        ExpensiveResource = new(CreateAndInitializeExpensiveResourceAsync);
    }

    public Task<object> GetExpensiveResourceAsync() => ExpensiveResource.Value;
}