2

The problem is that I'm trying to run RunPrivateMethod() multiple times but I'm running into blocking problems or just straight up not working when I use async/await. Rather than sharing every attempt here, I'm just putting my current version.

In RunPrivateMethod(), exeProcess.WaitForExit() method is an external program that basically reads/writes/crunches/outputs data. I tried to run this as an async task and it did not work.

I don't know if it's rational concern, but I want to limit the number of Tasks that get launched at one time so I put in Task.WaitAll() at the end of each case block. Case (1) and Case (2) always will both get run.

So, here is my code. Currently, it blocks as it loads up each task. Then only the last task seems to run. It works properly if I take out all the Task statements and run everything normally.

I would really and truly appreciate any input or help on this. Most of my tests end up locking up my system and keyboard.

public void RunSomeTasks(int group)
    {

        switch (group)
        {

            case (1):
                {
                    Task.Run(async () => await RunAsyncMethod(param1, param1, group));
                    Task.Run(async () => await RunAsyncMethod(param1, param2, group));
                    Task.Run(async () => await RunAsyncMethod(param1, param3, group));
                    Task.WaitAll();
                    break;
                }
            case (2):
                {
                    Task.Run(async () => await RunAsyncMethod(param2, param1, group));
                    Task.Run(async () => await RunAsyncMethod(param2, param2, group));
                    Task.Run(async () => await RunAsyncMethod(param2, param3, group));
                    Task.WaitAll();
                    break;
                }
        }
    }

    async Task RunAsyncMethod(string var1, string var2, string varGroup)
    {           

        ////////////////////////////////////////////
        // Use ProcessStartInfo class
        ProcessStartInfo startInfo = new ProcessStartInfo();
        startInfo.CreateNoWindow = false;
        startInfo.UseShellExecute = false;
        startInfo.FileName = "SomeOutsideEXE";
        startInfo.WindowStyle = ProcessWindowStyle.Hidden;
        startInfo.Arguments = var1 + " " + var2 + " " + varGroup;
        using (Process exeProcess = Process.Start(startInfo))
        {
            // did not work -> Task.Run(() => exeProcess.WaitForExit()).Wait();
            exeProcess.WaitForExit();
        }
    }
}

I have worked countless hours on this and read Cleary's book and this latest revision is a version of this post: Aync/Await action within Task.Run() The current results are that the last task in each set works although exeProcess is launched the correct amount of time. I cannot use the keyboard while it's running.

I did obviously try a straight async method for RunSomeTasks() and then just awaited RunAsyncMethod first. I seriously could use some help and, yep, I already know that I don't know what the heck I'm doing in spite of long hours of reading and trial and error.

Missy
  • 1,286
  • 23
  • 52
  • 1
    When you are in `RunSomeTasks`, it will block at this point `Task.WaitAll();` – CodingYoshi Oct 12 '18 at 03:34
  • If I take it out, I can't limit the number of times the task will run. I'd need something like await Task.WaitAll() to avoid the blocking while still limiting the number of times the Tasks run. – Missy Oct 12 '18 at 03:38
  • 2
    I get that but the thread will be blocked. 1) What kind of app (console, winform etc.) Is this? 2) What is the issue? 3) What do you want to do, achieve? – CodingYoshi Oct 12 '18 at 03:41
  • This is a winform app. The user launches the process and really doesn't have any work after that but they won't understand if their keyboard locks up. I just tried it with taking out the .waitall commands and it only ran the last task in the last set. The issue is that all the tasks aren't completing and the keyboard is locking up. – Missy Oct 12 '18 at 03:43
  • 2
    Make `RunSomeTasks` async and make the event handler which calls this method `async` too. That way the main thread (UI) will be around to react to user clicks etc. – CodingYoshi Oct 12 '18 at 03:47
  • I will try that. I tried it before and I got no results at all but something may have been different. – Missy Oct 12 '18 at 03:48
  • 1
    You should first make your *RunAsyncMethod* really async. The current version is **not async** at all and you should get a warning from compiler about that – Sir Rufo Oct 12 '18 at 03:54
  • Really good point. I will await exeProcess.WaitForExit(); – Missy Oct 12 '18 at 03:55
  • 2
    You cannot await exeProcess.WaitForExit. But you can use a TaskCompletionSource combined with [Process.Exited](https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.process.exited?view=netframework-4.7.2) event – Sir Rufo Oct 12 '18 at 03:55
  • Thanks for saving me time on that. I will try one of these solutions: https://stackoverflow.com/questions/10788982/is-there-any-async-equivalent-of-process-start#10789196 – Missy Oct 12 '18 at 04:01
  • 1
    Just make it work so in `RunAsyncMethod` you can put `await Task.Delay(3000)` for now and comment out the rest of the code. Once you get it going, then make it async. You may have to wrap all your code in Task.Run in this method if there is no true asynchronous work. Don't forget to do what I suggested in previous comments as well. Just get the flow working and then go to details. – CodingYoshi Oct 12 '18 at 04:08
  • Good idea. I will take everyone suggestions and rework it a bit and see what I can do. Thank you. I will post the results after work tomorrow. – Missy Oct 12 '18 at 04:17
  • Possible duplicate of [Limiting the amount of concurrent tasks in .NET 4.5](https://stackoverflow.com/questions/20355931/limiting-the-amount-of-concurrent-tasks-in-net-4-5) –  Oct 12 '18 at 04:25
  • 3
    It sounds like your tasks are IO bound. **Do not move IO bound tasks onto worker threads**. Keep them on the UI thread and use async IO. Otherwise what you are doing is hiring workers and then paying them to sleep! – Eric Lippert Oct 12 '18 at 06:27

2 Answers2

4

I modified your example a little, this should keep the UI from locking up. Note, I added async to the button click. Also using WhenAll and passing in the started tasks, instead of using WaitAll. (Updated for full async mode)

    private async void button1_Click(object sender, EventArgs e)
    {
        try
        {
            await RunSomeTasks(1);
            await RunSomeTasks(2);
            lblStatus.Text = "done!";
        }
        catch (Exception ex)
        {
            MessageBox.Show(ex.ToString());
        }
    }

    public async Task RunSomeTasks(int group)
    {
        switch (group)
        {
            case (1):
                {
                    var t1 = RunMethodAsync(param1, param1, group);
                    var t2 = RunMethodAsync(param1, param2, group);
                    var t3 = RunMethodAsync(param1, param3, group);
                    await Task.WhenAll(t1, t2, t3).ConfigureAwait(false);
                    break;
                }
            case (2):
                {
                    var t1 = RunMethodAsync(param2, param1, group);
                    var t2 = RunMethodAsync(param2, param2, group);
                    var t3 = RunMethodAsync(param2, param3, group);
                    await Task.WhenAll(t1, t2, t3).ConfigureAwait(false);
                    break;
                }
        }
    }

    async Task RunMethodAsync(string var1, string var2, int varGroup)
    {
        ////////////////////////////////////////////
        // Use ProcessStartInfo class
        ProcessStartInfo startInfo = new ProcessStartInfo();
        startInfo.CreateNoWindow = false;
        startInfo.UseShellExecute = false;
        startInfo.FileName = "SomeOutsideEXE";
        startInfo.WindowStyle = ProcessWindowStyle.Hidden;
        startInfo.Arguments = var1 + " " + var2 + " " + varGroup;

        using (Process exeProcess = new Process())
        {
            var cts = new TaskCompletionSource<int>();

            exeProcess.StartInfo = startInfo;
            exeProcess.EnableRaisingEvents = true;

            exeProcess.Exited += (sender, e) =>
            {
                try
                {
                    cts.SetResult(exeProcess.ExitCode);
                }
                catch (Exception ex)
                {
                    cts.SetException(ex);
                }
            };

            exeProcess.Start();

            await cts.Task.ConfigureAwait(false);
        }
    }
C. Mitchell
  • 176
  • 7
  • Thank you so much for writing this up. I will try it after work tomorrow and let you know how it works. – Missy Oct 12 '18 at 04:21
  • 2
    When Stephen Cleary is mentioned then you should never forget *ConfigureAwait(false)* when and where possible :o) – Sir Rufo Oct 12 '18 at 05:44
  • 1
    Probably needs calcification why Shouldn't `await cts.Task` be inside `exeProcess.Exited ` handler? Else this answer is good. – CodingYoshi Oct 12 '18 at 06:07
  • 2
    In fact, you want `ConfigureAwait(true)` here, because you want to continue on the ui thread because otherwise `lblStatus.Text = "done!"` will fail. – Haukinger Oct 12 '18 at 11:16
  • 2
    @CodingYoshi, if you put the await cts.Task inside the exeProcess.Exited handler, the RunMethodAsync method will not await the cts.Task before exiting the using block. Granted you'll still launch the processes but, you'll lose any ability to await on the event. – C. Mitchell Oct 12 '18 at 12:40
  • OMG -- It's looking really good!!! Thanks everyone so much. I will finish testing after work and report back/accept this answer. I could have never done it without you! – Missy Oct 12 '18 at 14:42
  • 1
    @SirRufo, good point on the ConfigureAwait(false). Could also add that to the cts.Task: `var exitCode = await cts.Task.ConfigureAwait(false);`. – C. Mitchell Oct 12 '18 at 15:16
  • 2
    @Haukinger, I think putting the ConfigureAwait(false) on the `WhenAll` and `cts.Task` doesn't change the continuation context of `await RunSomeTasks(1)`. In other words, if we don't put ConfigureAwait(false) on the call to RunSomeTasks(1) then when that continues it will continue on the UI context regardless of if we put ConfigureAwait(false) within the RunSomeTasks(1) method or further up the stack. – C. Mitchell Oct 12 '18 at 15:21
  • 1
    @C.Mitchell you're right. For clarity's sake, you could add `ConfigureAwait(true)` to `RunSomeTasks(1)`. – Haukinger Oct 12 '18 at 15:26
  • So super close - thank you all so much for your help. One thing is not working properly. I issue: await RunSomeTasks(1); then await RunSomeTasks(2); and the first one works and then it exits the program. Any clue? I could not trace it. I tried ConfigureAwait() with both true and false but I don't have the understanding to put it together. Help! – Missy Oct 13 '18 at 01:14
  • 1
    @Missy I'm not able to reproduce it using the sample but, it sounds like you're getting an exception somewhere which is terminating the application. I've updated the sample to include a couple of try/catch blocks, added a ConfigureAwait to the cts.Task await and added in a call to RunSomeTasks(2). You'll need to see what is throwing the exception which the try/catch block and messagebox can help with. – C. Mitchell Oct 13 '18 at 04:42
  • 1
    I was wondering why not wrap the calls to `RunSomeTasks` in `button1_Click` in a single `Task.Run()` so that you don't need the 3x `ConfigureAwait(false)`? – pere57 Oct 13 '18 at 05:06
  • @C.Mitchell Thank you so much for your help. I will integrate them and let you know how I do. – Missy Oct 13 '18 at 18:22
  • 1
    @pere57, You could wrap it in a `Task.Run()` and that would work but, since `RunSomeTasks()` is not CPU bound it's not [ideal](https://blog.stephencleary.com/2013/10/taskrun-etiquette-and-proper-usage.html) as it would tie up a thread until all of the started processes have exited. For this example, you could also take out `ConfigureAwait(false)` and that too would work but, that is not a [best practice](https://msdn.microsoft.com/en-us/magazine/jj991977.aspx). Using `ConfigureAwait(false)` helps avoid deadlocks and in keeping the UI from becoming sluggish. – C. Mitchell Oct 13 '18 at 18:47
  • @C.Mitchell - 1. Thank you so much and I really and truly appreciate your help. 2. I got it working by changing the signature from async Task RunMethodAsync to private Task RunMethodAsync Can you (or anyone) give me thoughts about why that would work and if it makes sense? Sadly, async is at a level of abstraction that my brain seems to be incapable of at this time in spite of a lot of work on it :( – Missy Oct 13 '18 at 20:38
  • 1
    @Missy, without seeing what the method looks like now it's difficult to say but, my guess is that something was causing the exception to be thrown from RunMethodAsync and terminating the application. With the method no longer async, the await must also not be there and it could be that the method is returning the cts.Task immediately. This would effectively be a fire and forget on the spawned processes and the application woudn't know when they exited. Unless you're using WaitForExit and not relying on the Exited event? – C. Mitchell Oct 13 '18 at 22:01
  • 1
    @C.Mitchell -- Not sure how to thank you but I upvoted you so you should at least see some new points and I accepted your answer. You helped me on an epic problem and I will pay it forward somehow to be sure :) – Missy Oct 13 '18 at 23:22
2

The cleanest way I have come up with to do an await Process.WaitForExit is like this, saves subscribing to events etc.

private async Task RunRubyScript(string filePath)
{
  await Task.Run(() =>
  {
    using (var proc = new Process())
    {
      var startInfo = new ProcessStartInfo(@"ruby");
      startInfo.Arguments = filePath;
      startInfo.UseShellExecute = false;
      startInfo.CreateNoWindow = true;
      proc.StartInfo = startInfo;
      proc.Start();
      proc.WaitForExit();
    }
  });
}

This was to run a ruby script, however obviously modify you ProcessStartInfo accordingly to match your requirements.

Hope this helps!

Tristan Trainer
  • 2,770
  • 2
  • 17
  • 33
  • This is a good idea, Tristan. I've used something like that before but not in this form. I like it :) – Missy Apr 08 '19 at 17:26