0

I am a newbie and trying to learn the right way.

Is it acceptable to use a Thread within a constructor to avoid gui(Form) to freeze when an object is created? I will reuse this class often.

class Cmd
{
    protected static string parameters;
    protected HashSet<string> list_result;

    public Cmd( string parameters)
    {
        Thread Thread1 = new Thread(new ThreadStart(Process1));
        Thread1.Start();
        Thread1.Join();
    }

     private void Process1()
    {
        ProcessStartInfo processStartInfo = new ProcessStartInfo("cmd", "/c " + parameters);
        processStartInfo.RedirectStandardOutput = true;
        processStartInfo.RedirectStandardError = true;
        processStartInfo.CreateNoWindow = true;
        processStartInfo.UseShellExecute = false;

        Process process = Process.Start(processStartInfo);

        list_result = new HashSet<string>();
        while (!process.StandardOutput.EndOfStream)
        {
            string line = process.StandardOutput.ReadLine();
            list_result.Add(line);
        }
    }
Improve this question
5
  • 5
    The way you've written the code here won't help one bit since you join on the thread, effectively blocking until the thread is done. Did this actually solve anything? Commented Jun 10, 2015 at 11:54
  • I wouldn't join on the thread - not in the constructor. Can't you use a continuation, at least? You'd need to use a task rather than your System.Thread, but for what you're trying to do, that should be fine, IMHO... Commented Jun 10, 2015 at 11:55
  • Instead of a separate class, consider using Task.Run to run your method. You can call await in your UI to await asynchronously for the other process to finish. Commented Jun 10, 2015 at 12:06
  • Or you could use ReadLineAsync instead of ReadLine and avoid blocking without using a thread at all Commented Jun 10, 2015 at 12:08
  • It works fine and I only wanted to know if this practice is acceptable. join() is needed since the object has to be created before any methods are called. As advice, I will try Task since it looks simple to implement. Commented Jun 10, 2015 at 12:53

2 Answers 2

Reset to default
3

You don't even need a thread for this. You can use the StreamReader's asynchronous methods to read the input lines asynchronously: 

    private async void button1_Click(object sender, EventArgs e)
    {
        var lines=await Process1(@"dir g:\ /s");
        var result= String.Join("|", lines);
        this.textBox1.Text = result;
    }

    private async Task<HashSet<String>>   Process1(string parameters)
    {
        var list_result = new HashSet<string>();
        ProcessStartInfo processStartInfo = new ProcessStartInfo("cmd", "/c " + parameters);
        processStartInfo.RedirectStandardOutput = true;
        processStartInfo.RedirectStandardError = true;
        processStartInfo.CreateNoWindow = true;
        processStartInfo.UseShellExecute = false;

        Process process = Process.Start(processStartInfo);


        while (!process.StandardOutput.EndOfStream)
        {
            string line = await process.StandardOutput.ReadLineAsync();
            list_result.Add(line);
        }
        return list_result;
    }

The advantage is that you don't waste a thread, you don't need any synchronization code or static fields to pass the parameters and read the results.

Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

0

If you would like to avoid freeze in your UI when doing a time-consuming task, you should add BackgroundWorker to your form and run your task in its event handler

You can find the example here: https://msdn.microsoft.com/en-us/library/cc221403%28v=vs.95%29.aspx

You should also consider using newer async/await logic which is usually better than BackgroundWorker, as Panagiotis Kanavos mentioned in his answer

Improve this answer

6 Comments

Task.Run and async/await are easier and allow composing multiple asynchronous calls, exception handling etc. BackgroundWorker is useful only for compatibility reasons
@Panagiotis Kanavos - agreed, these can be used as well.
@Panagiotis Kanavos Why do you say those options are easier than BackgroundWorker? I've found it easy to use while at least async/await seems fairly confusing. Maybe I just haven't read the right documentation.
@bkribbs check my answer to see how this can be done using async/await without any threading at all. In fact, BackgroundWorker makes it almost impossible to perform two separate asynchronous actions (eg download a file and write it to a database, or make two web service calls) - you'd need two separate workers, or perform everything synchronously.
@bkribbs: I have a blog post series on using Task.Run instead of BackgroundWorker.
|

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.