-1

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?

Improve this question
4
  • 1
    What output did you expect? Commented Oct 16, 2020 at 18:22
  • 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. Commented Oct 16, 2020 at 18:25
  • 1
    Why do you use Task.Run? You can just await a Task.Delay(1000). No need to run another task and block it. Commented Oct 16, 2020 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) Commented Oct 16, 2020 at 18:33

2 Answers 2

Reset to default
3

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 } } }
Improve this answer
Sign up to request clarification or add additional context in comments.

2 Comments

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.
@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.
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.

Improve this answer

6 Comments

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.
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.learn.microsoft.com/en-us/dotnet/csharp/programming-guide/…
@Myntekt If you want it to run concurrently, do not await Task.Run.
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.
@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
|

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.