Problems when trying to use async/await

Question

I'm following this thread to make some of my commands Async while others sync but I'm having some trouble.

This is what I have so far.

An interface:

public interface ICommand
{
    public Task ExecuteAsync();
}

Which I implement in a Wait Command which should be synchronous.

public class WaitCommand : ICommand
{
    public Task ExecuteAsync()
    {
        Console.WriteLine("Sleeping...");
        Thread.Sleep(5000);
        Console.WriteLine("Waking up...");
        return Task.CompletedTask;
    }
}

And another command that does work and should be assynchronous.

public class ConcreteCommand : ICommand
{
    public async Task ExecuteAsync()
    {
        await Task.Run(() =>
        {
            Console.WriteLine("Starting...");
            Thread.Sleep(1000);
            Console.WriteLine("Ending...");
        });
    }
}

Then I have some sort of method to read a string and convert it to a command:

public async Task ExecuteAsync(string commandName)
    {
        ... get command

        await command.ExecuteAsync();
    }

And finally I have a loop

List<string> commands = new List<string>
{
    "concreteCommand",
    "wait",
    "concreteCommand"
};

foreach (String command in commands)
{
    await commandDispatcher.ExecuteAsync(command);
}

This is what it prints.

Starting...
Ending...
Sleeping...
Waking up...
Starting...
Ending...

Which means it's not running asynchronously. What's wrong in my implementation?

Starting... Sleeping.. Ending... Waking up... Starting... Ending... Basically it should keep starting new commands until a wait. After waking up, it should keep adding the remaning commands. Commands that are executing should keep going. — Lucas, Oct 16 '20 at 18:25
Why do you use Task.Run? You can just await a Task.Delay(1000). No need to run another task and block it. — insane_developer, Oct 16 '20 at 18:31
The sleeping in the ConcreteCommand is a simulation of a long task. What other option do I have (assuming that in a normal scenario I do not want to do sleep but do something useful) — Lucas, Oct 16 '20 at 18:33

score 3 · Accepted Answer · answered Oct 17 '20 at 00:20

Though there is nothing asynchronous in Thread.Sleep(1000) the reason for described in question behavior is in the await inside the foreach statement:

foreach (String command in commands)
{
    await commandDispatcher.ExecuteAsync(command);
}

You basically starting and waiting for end of each command before proceeding with next. Change it to:

await Task.WhenAll(commands.Select(command => commandDispatcher.ExecuteAsync(command)));

And you will get output close to expected in the comments (though order of first two statements can differ).

Note that WaitCommand in this case will prevent next commands from starting. If that is the goal I would say that from performance standpoint it would be better to refactor code somehow so you would not consume thread and CPU just for wait. For example something like this (example code, maybe some fixes will be needed to make it compile):

List<string> commands = new List<string>
{
    "concreteCommand",
    "wait",
    "concreteCommand"
};

await commandDispatcher.ExecuteAsync(commands);

// Dispatcher 
public async Task ExecuteAsync(IEnumerable<string> commands)
{
   foreach (String commandName in commands)
   {
       //... get command
       if(command is WaitCommand)
       {
          await Task.Delay(5000); // or read from `WaitCommand` props
          // or use Task.Delay in WaitCommand and await command here
       }
       else
       {
          _ = command.ExecuteAsync(); // fire and forget which is usually not recommended
          // or add to some collection and await Task.WhenAll(...) after the loop
       }
   }
}

I guess this sort of works. But then it means that the dispatcher has to be aware of the possible command implementations which is not ideal. The idea is that the dispatcher knows then interface but has no idea what will actually be executed. — Lucas, Oct 17 '20 at 00:26
@Myntekt it means that dispatcher will be aware of one special case command type. Otherwise you can exhaust ThreadPool and possibly have performance problems if you have a lot of simultaneous waits in your app. Of cause you need to try and measure and maybe in your case your approach will not introduce any issues. — Guru Stron, Oct 17 '20 at 00:35

score 0 · Answer 2 · answered Oct 16 '20 at 18:32

0

public class ConcreteCommand : ICommand
{
    public async Task ExecuteAsync(List<string> arguments)
    {
        // this await causes the caller of ExecuteAsync
        // to wait for the result of Task.Run
        await Task.Run(() =>
        {
            Console.WriteLine("Starting...");
            Thread.Sleep(1000);
            Console.WriteLine("Ending...");
        });
    }
}

BTW, Thread.Sleep is super old school and evil. It ties up the thread, and threads are expensive resources. This confuses the thread pool about how many threads it has available and forces it to make new ones. Use await Task.Delay instead.

answered Oct 16 '20 at 18:32

StackOverthrow

1,158
11
23

Concrete command uses Thread.Sleep(1000) to simulate a real command that takes a while to execute. It should run asynchronously with other tasks. Wait command uses Thread.Sleep(5000) because it should keep other commands from being initialize. I can't use await Task.Delay because the whole point of wait is to keep it from continuing. – Lucas Oct 16 '20 at 18:39
I am afraid you do not understand what await does given your comment above. Better read the excellent documentation. Not to be mean but it does help I think.https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/concepts/async/ – Peter Bons Oct 16 '20 at 18:42
1

@Myntekt If you want it to run *concurrently*, do not await Task.Run. – StackOverthrow Oct 16 '20 at 18:53
1

The await key keyword stops the current execution and waits at this point for the return from an asnyc function/method (so this execution thread will not block other threads). As your ConcreteCommand does not return any value, you may not need to await it's return. But maybe you take the hint from @PeterBons, async does not necessarily means parallel execution. – Danny Schneider Oct 16 '20 at 19:41
1

@PeterBons think we misunderstood, I wanted to tell Myntekt that he should read your suggested tutorial, since he apparently expects await / async to run in parallel in general. I fully agree with you – Danny Schneider Oct 16 '20 at 20:30
@dannyschneider my bad, I did misread your comment – Peter Bons Oct 17 '20 at 09:14

Problems when trying to use async/await

2 Answers2