12
class Laziness
{
    static string cmdText = null;
    static SqlConnection conn = null;

 
    Lazy<Task<Person>> person =
        new Lazy<Task<Person>>(async () =>      
        {
            using (var cmd = new SqlCommand(cmdText, conn))
            using (var reader = await cmd.ExecuteReaderAsync())
            {
                if (await reader.ReadAsync())
                {
                    string firstName = reader["first_name"].ToString();
                    string lastName = reader["last_name"].ToString();
                    return new Person(firstName, lastName);
                }
            }
            throw new Exception("Failed to fetch Person");
        });

    public async Task<Person> FetchPerson()
    {
        return await person.Value;              
    }
}

And the book, "Concurrency in .NET" by Riccardo Terrell, June 2018, says:

But there's a subtle risk. Because Lambda expression is asynchronous, it can be executed on any thread that calls Value and the expression will run within the context. A better solution is to wrap the expression in an underlying Task which will force the asynchronous execution on a thread pool thread.

I don't see what's the risk from the current code ?

Is it to prevent deadlock in case the code is run on the UI thread and is explicity waited like that:

new Laziness().FetchPerson().Wait();
Johann
  • 4,107
  • 3
  • 40
  • 39
John
  • 4,351
  • 9
  • 41
  • 57
  • Even if the underlying task was run on a different thread, that `Wait()` would still block the UI for the duration of the task... – Patrick Roberts Aug 16 '19 at 00:37
  • 2
    There is currently no risk in this code, However it is suggesting to offload the async lambda to a threadpool thread, which i think in this case is overkill and a waste of a thread – TheGeneral Aug 16 '19 at 00:46
  • @TheGeneral a thread would be wasted only in case there was synchronous code inside `cmd.ExecuteReaderAsync`, which is highly improbable. The expected scenario is that the thread will execute a handful of CPU instructions and then return back to the thread-pool. – Theodor Zoulias Aug 16 '19 at 13:22
  • 1
    Why are you even using lazy? Just make a function and call it when you need it? – johnny 5 Aug 16 '19 at 14:18
  • https://devblogs.microsoft.com/pfxteam/asynclazyt/ that is from 2011. Eight years before. – Chef Gladiator Jul 29 '22 at 21:57

2 Answers2

10

I don't see what's the risk from the current code ?

To me, the primary issue is that the asynchronous initialization delegate doesn't know what context/thread it'll run on, and that the context/thread could be different based on a race condition. For example, if a UI thread and a thread pool thread both attempt to access Value at the same time, in some executions the delegate will be run in a UI context and in others it will be run in a thread pool context. In the ASP.NET (pre-Core) world, it can get a bit trickier: it's possible for the delegate to capture a request context for a request that is then canceled (and disposed), and attempt to resume on that context, which isn't pretty.

Most of the time, it wouldn't matter. But there are these cases where Bad Things can happen. Introducing a Task.Run just removes this uncertainty: the delegate will always run without a context on a thread pool thread.

Stephen Cleary
  • 437,863
  • 77
  • 675
  • 810
  • Hey @Stephen, great answer, why did you mentioned the "pre-Core" parenthesis? – Shahar Shokrani Apr 01 '20 at 15:21
  • 2
    Because ASP.NET doesn't have an ambient request context. Pre-Core ASP.NET did, so there's a problem where the context could be disposed and then the code tries to use it. – Stephen Cleary Apr 01 '20 at 21:07
  • Is it possible that you can have issues with the current culture? Suppose in ASP.NET (pre-Core) where each request is in a different language/culture (depending on the current user), then this can throw an exception in the current culture, causing the exception to be localized. Or other issues with functions depending on the culture (parsing/formatting) – Christophe Devos Oct 25 '21 at 11:33
  • @ChristopheDevos: Yes, that's possible. My `AsyncLazy` type [includes a flag](https://github.com/StephenCleary/AsyncEx/blob/0361015459938f2eb8f3c1ad1021d19ee01c93a4/src/Nito.AsyncEx.Coordination/AsyncLazy.cs#L22) that avoids the `Task.Run` for this reason. – Stephen Cleary Oct 26 '21 at 13:02
  • https://devblogs.microsoft.com/pfxteam/asynclazyt/ that is from 2011. Eight years before. – Chef Gladiator Jul 29 '22 at 21:56
  • @ChefGladiator: I don't understand what your comment is attempting to say. – Stephen Cleary Jul 29 '22 at 21:59
  • @StephenCleary is that not AsyncLazy, predating your AsyncLazy for eight years? I might be mistaken but it looks like the nucleus of your solution. – Chef Gladiator Jul 30 '22 at 05:58
  • The question was regarding an async lazy that the op got from a book, which may have been (probably was) influenced by that blog post. My answer clarified the concern mentioned by the book author. So I still don't understand the relevance of the age of that blog post. – Stephen Cleary Jul 30 '22 at 11:09
9

I simplified your example to show what happens in each case. In the first case the Task is created using an async lambda:

Lazy<Task<string>> myLazy = new Lazy<Task<string>>(async () =>
{
    string result = $"Before Delay: #{Thread.CurrentThread.ManagedThreadId}";
    await Task.Delay(100);
    return result += $", After Delay: #{Thread.CurrentThread.ManagedThreadId}";
});

private async void Button1_Click(object sender, EventArgs e)
{
    int t1 = Thread.CurrentThread.ManagedThreadId;
    var result = await myLazy.Value;
    int t2 = Thread.CurrentThread.ManagedThreadId;
    MessageBox.Show($"Before await: #{t1}, {result}, After await: #{t2}");
}

I embedded this code in a new Windows Forms application with a single button, and on clicking the button this message popped up:

Before await: #1, Before Delay: #1, After Delay: #1, After await: #1

Then I changed the valueFactory argument to use Task.Run instead:

Lazy<Task<string>> myLazy = new Lazy<Task<string>>(() => Task.Run(async () =>
{
    string result = $"Before Delay: #{Thread.CurrentThread.ManagedThreadId}";
    await Task.Delay(100);
    return result += $", After Delay: #{Thread.CurrentThread.ManagedThreadId}";
}));

Now the message is this:

Before await: #1, Before Delay: #3, After Delay: #4, After await: #1

So not using Task.Run means that your code before, between and after the awaits will run on the UI thread. Which might not be a big deal, unless there is CPU intensive or IO blocking code hidden somewhere. For example the constructor of the Person class, as innocent as it might look, could contain some call to a database or web API. By using Task.Run you can be sure that the initialization of the Lazy class will not touch the UI thread before it's done.

Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104