none
How can I ensure that all process output is captured when using Process.BeginOutputReadLine? RRS feed

  • Question

  • I'm using the following code (based on an MSDN sample) to capture output and error streams of a child process; however, this code has a race condition that makes it lose data intermittently. I've added 2 sleeps to make this error happen more often.

    Any ideas how to avoid this race condition and make sure that all pending async stream operations are finished?

    	class Program
    	{
    		static void Main()
    		{
    			for (int i = 0; i < 1000; i++)
    			{
    				var res = RunTest();
    				if (res.Trim() != "TEST")
    					Debugger.Break(); // should never break here
    			}
    		}
    
    		static string RunTest()
    		{
    			var info = new ProcessStartInfo(CmdPath, " /C echo TEST")
    			{
    				CreateNoWindow = true,
    				UseShellExecute = false,
    				RedirectStandardInput = true,
    				RedirectStandardOutput = true,
    				RedirectStandardError = true,
    			};
    
    			var process = Process.Start(info);
    
    			var sync = new object();
    			var buf = new StringBuilder();
    
    			process.OutputDataReceived +=
    				(sender, args) =>
    				{
    					Thread.Sleep(1);
    					lock (sync)
    					{
    						buf.AppendLine(args.Data);
    						Trace.WriteLine(args.Data);
    					}
    				};
    			process.BeginOutputReadLine();
    
    			process.ErrorDataReceived +=
    				(sender, args) =>
    				{
    					Thread.Sleep(1);
    					lock (sync)
    					{
    						buf.AppendLine(args.Data);
    						Trace.WriteLine(args.Data);
    					}
    				};
    			process.BeginErrorReadLine();
    
    			if (!process.WaitForExit(10000))
    				throw new ApplicationException();
    
    			lock (sync)
    			{
    				var res = buf.ToString();
    				return res;
    			}
    		}
    
    		static string CmdPath => Path.Combine(Environment.SystemDirectory, "cmd.exe");
    	}
    


    • Edited by Andrey_F Monday, June 20, 2016 8:49 AM
    Monday, June 20, 2016 8:43 AM

Answers

  • So, the root of this problem must be here:

            public bool WaitForExit(int milliseconds)
            {
                Microsoft.Win32.SafeHandles.SafeProcessHandle processHandle = null;
                bool flag;
                ProcessWaitHandle handle2 = null;
                try
                {
                    processHandle = this.GetProcessHandle(0x100000, false);
                    if (processHandle.IsInvalid)
                    {
                        flag = true;
                    }
                    else
                    {
                        handle2 = new ProcessWaitHandle(processHandle);
                        if (handle2.WaitOne(milliseconds, false))
                        {
                            flag = true;
                            this.signaled = true;
                        }
                        else
                        {
                            flag = false;
                            this.signaled = false;
                        }
                    }
                }
                finally
                {
                    if (handle2 != null)
                    {
                        handle2.Close();
                    }
                    if ((this.output != null) && (milliseconds == -1))
                    {
                        this.output.WaitUtilEOF();
                    }
                    if ((this.error != null) && (milliseconds == -1))
                    {
                        this.error.WaitUtilEOF();
                    }
                    this.ReleaseProcessHandle(processHandle);
                }
                if (flag && this.watchForExit)
                {
                    this.RaiseOnExited();
                }
                return flag;
            }

    This method calls WaitUtilEOF() methods on process output streams, when the WaitForExit() method is called and it successfully awaits for the process exit; however, WaitUtilEOF() methods are not called when WaitForExit(int milliseconds) is called and successfully awaits for the process exit.

    Looks like a bug in BCL.


    And a more correct workaround for this bug:

    		}
    		// .............
    
                            if (!process.WaitForExit(10000))
                            {
                                process.Kill();
                                throw new ApplicationException();
                            }
                            process.WaitForExit();
    lock (sync) { var res = buf.ToString(); return res; } }






    • Proposed as answer by DotNet WangModerator Tuesday, June 21, 2016 11:31 AM
    • Marked as answer by Andrey_F Tuesday, June 21, 2016 12:21 PM
    • Edited by Andrey_F Tuesday, June 21, 2016 12:23 PM
    Tuesday, June 21, 2016 9:56 AM

All replies

  • Hi Andrey_F,

    >>"this code has a race condition that makes it lose data intermittently. "

    The reason is that you didn't wait until OutputDataReceived and ErrorDataReceived executed sucess.

    lock (sync) { var res = buf.ToString();
    //res here always be empty string return res; }

    You could declare 2 variables which representative of whether the two methods have bee execute completed. Code below is for your reference.

    bool outputDataReceived = false;
    bool errorDataReceived = false;
    
    process.OutputDataReceived +=
    	(sender, args) =>
    	{
    		Thread.Sleep(1);
    		lock (sync)
    		{
    			buf.AppendLine(args.Data);
    			Trace.WriteLine(args.Data);
    		}
                    outputDataReceived = true;
    	};
    
    process.BeginOutputReadLine();
    
    process.ErrorDataReceived +=
    	(sender, args) =>
    	{
    		Thread.Sleep(1);
    		lock (sync)
    		{
    			buf.AppendLine(args.Data);
    			Trace.WriteLine(args.Data);
    		}
                    errorDataReceived = true;
    	};
    
    process.BeginErrorReadLine();
    
    while (true)
    {
        if (outputDataReceived && errorDataReceived)
        {
            return buf.ToString();
        }
    }
    Best Regards,
    Li Wang

    We are trying to better understand customer views on social support experience, so your participation in this interview project would be greatly appreciated if you have time. Thanks for helping make community forums a great place.
    Click HERE to participate the survey.

    Tuesday, June 21, 2016 2:24 AM
    Moderator
  • lock (sync)

    { var res = buf.ToString();
    //res here always be empty string return res; }

    You could declare 2 variables which representative of whether the two methods have bee execute completed. Code below is for your reference.

    > res here always be empty string

    Not really.

    Also see this sample in MSDN https://msdn.microsoft.com/en-us/library/system.diagnostics.process.beginoutputreadline(v=vs.110).aspx

    > You could declare 2 variables which representative of whether the two methods have bee execute completed. Code below is for your reference.

    This code is completely non-functional.

    1. Here you are setting and reading a variable from different threads without proper sync primitives, and this won't work correctly.

    2. If this code worked, it would only guaranteed that one and only one portion of process output data of each kind is received; however, both the standard output (and error output) of the process can contain more than one portion, or no output at all.

    Tuesday, June 21, 2016 3:16 AM
  • Hi Andrey_F,

    "> res here always be empty string

    Not really."

    Of course, if you set a breakpoint in RunTest method. The res may have value.

    >>"Here you are setting and reading a variable from different threads without proper sync primitives, and this won't work correctly. "

    One variable is only assigned in one thread. For example, outputDataReceived only assigned in OutputDataReceived  thread. It will not cause conflicts with shared resources.

    >>"If this code worked, "

    I tested it on my side and it worked fine.

    >>"however, both the standard output (and error output) of the process can contain more than one portion, or no output at all."

    Whether standard and error provide output or not,both OutputDataReceived and ErrorDataReceived methods will be raised. 

    Best Regards,
    Li Wang


    We are trying to better understand customer views on social support experience, so your participation in this interview project would be greatly appreciated if you have time. Thanks for helping make community forums a great place.
    Click HERE to participate the survey.

    Tuesday, June 21, 2016 3:31 AM
    Moderator
  • Hi Andrey_F,

    > Of course, if you set a breakpoint in RunTest method. The res may have value.

    It has correct value in 99.999% cases when the Thread.Sleep calls are removed. And the sample in MSDN is doing this in exactly the same way. So I assumed that it's the officially recommended way how this task should be done.

    > One variable is only assigned in one thread. For example, outputDataReceived only assigned in OutputDataReceived  thread. It will not cause conflicts with shared resources.

    This variable is assigned in a background thread that invokes callbacks, and is read from the main thread. This code is not thread safe and incorrect due to possible compiler/runtime optimizations and partial read/write problems.

    > Whether standard and error provide output or not,both OutputDataReceived and ErrorDataReceived methods will be raised.

    Even if the lunar phase is favorable and this code doesn't fail, it will handle only a single portion of the incoming process output. And the output can come in multiple portions, exact number unknown.

    This can be checked very easily by replacing output args to " /C echo TEST & echo TEST & echo TEST"


    • Edited by Andrey_F Tuesday, June 21, 2016 5:29 AM
    Tuesday, June 21, 2016 5:29 AM
  • Hi Andrey_F,

    Sorry for misundertanding your question. You are right.

    After changing these lines of code

    if (!process.WaitForExit(10000))
        throw new ApplicationException();

    To

    process.WaitForExit();

    The code which you provided will work fine. Please try it.

    Best Regards,
    Li Wang


    We are trying to better understand customer views on social support experience, so your participation in this interview project would be greatly appreciated if you have time. Thanks for helping make community forums a great place.
    Click HERE to participate the survey.


    Tuesday, June 21, 2016 6:01 AM
    Moderator
  • After changing these lines of code
    if (!process.WaitForExit(10000))
        throw new ApplicationException();

    To

    process.WaitForExit();

    This seems to solve the problem, or at least I can't reproduce it quickly.

    Unfortunately, I need to use the WaitForExit overload with the timeout argument.

    Any ideas why one of these overloads works correctly and another doesn't? Is this a bug in BCL?

    • Edited by Andrey_F Tuesday, June 21, 2016 6:25 AM
    Tuesday, June 21, 2016 6:21 AM
  • Hi Andrey_F,

    >>"I need to use the WaitForExit overload with the timeout argument. "

    I tested following code, also worked fine. It could implement timeout feature for you.

    while (!process.HasExited)
    {
        Thread.Sleep(100);
    }

    >>"Any ideas why one of these overloads works correctly and another doesn't?"

    I have not found any differences between these method according MSDN document. You could use this workaround. If there are any updates, I will inform you timely.

    Best Regards,
    Li Wang


    We are trying to better understand customer views on social support experience, so your participation in this interview project would be greatly appreciated if you have time. Thanks for helping make community forums a great place.
    Click HERE to participate the survey.

    Tuesday, June 21, 2016 6:29 AM
    Moderator
  • while (!process.HasExited)
    {
        Thread.Sleep(100);
    }

    Solving threading problems using Sleeps is one of very, very bad practices. This code doesn't work if you replace 100 with 1, for example. It just hides the problem, makes it happen less often.

    • Edited by Andrey_F Tuesday, June 21, 2016 8:09 AM
    Tuesday, June 21, 2016 8:01 AM
  • So, the root of this problem must be here:

            public bool WaitForExit(int milliseconds)
            {
                Microsoft.Win32.SafeHandles.SafeProcessHandle processHandle = null;
                bool flag;
                ProcessWaitHandle handle2 = null;
                try
                {
                    processHandle = this.GetProcessHandle(0x100000, false);
                    if (processHandle.IsInvalid)
                    {
                        flag = true;
                    }
                    else
                    {
                        handle2 = new ProcessWaitHandle(processHandle);
                        if (handle2.WaitOne(milliseconds, false))
                        {
                            flag = true;
                            this.signaled = true;
                        }
                        else
                        {
                            flag = false;
                            this.signaled = false;
                        }
                    }
                }
                finally
                {
                    if (handle2 != null)
                    {
                        handle2.Close();
                    }
                    if ((this.output != null) && (milliseconds == -1))
                    {
                        this.output.WaitUtilEOF();
                    }
                    if ((this.error != null) && (milliseconds == -1))
                    {
                        this.error.WaitUtilEOF();
                    }
                    this.ReleaseProcessHandle(processHandle);
                }
                if (flag && this.watchForExit)
                {
                    this.RaiseOnExited();
                }
                return flag;
            }

    This method calls WaitUtilEOF() methods on process output streams, when the WaitForExit() method is called and it successfully awaits for the process exit; however, WaitUtilEOF() methods are not called when WaitForExit(int milliseconds) is called and successfully awaits for the process exit.

    Looks like a bug in BCL.


    And a more correct workaround for this bug:

    		}
    		// .............
    
                            if (!process.WaitForExit(10000))
                            {
                                process.Kill();
                                throw new ApplicationException();
                            }
                            process.WaitForExit();
    lock (sync) { var res = buf.ToString(); return res; } }






    • Proposed as answer by DotNet WangModerator Tuesday, June 21, 2016 11:31 AM
    • Marked as answer by Andrey_F Tuesday, June 21, 2016 12:21 PM
    • Edited by Andrey_F Tuesday, June 21, 2016 12:23 PM
    Tuesday, June 21, 2016 9:56 AM
  • Hi Andrey_F,

    Thanks for providing your workaround and  the community can benefit.

    Best Regards,
    Li Wang


    We are trying to better understand customer views on social support experience, so your participation in this interview project would be greatly appreciated if you have time. Thanks for helping make community forums a great place.
    Click HERE to participate the survey.

    Tuesday, June 21, 2016 11:31 AM
    Moderator