Console.ReadLine() is blocked in Linux even though the StandardInput of the process has been disposed

[daxian-dbw]

I'm working on a program that chains the standard output and standard input of a collection of processes, similar to the piping functionality in a shell. I use a worker thread to relay bytes between the output of upstream process and the input of the downstream process. When _upstream.StandardOutput.BaseStream.Read return 0, the worker thread dispose the StandardInput of the downstream process, and in that case, Console.ReadLine() in the downstream process is supposed to return null, so that the downstream process knows there is no more input and starts to wrap up and exit.

This works fine in Linux when 2 processes are chained, however, when more than 2 processes are chained, Console.ReadLine() in the downstream process continues to block even though the StandardInput of the process has been disposed.
However, the same code works fine on windows when chaining any numbers of processes.

Repro Code

worker-thread-pipe: work-thread.exe

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Runtime.InteropServices;
using System.Threading;

namespace ConsoleApplication
{
    public class Program
    {
        /// <summary>
        /// Number of child processes
        /// </summary>
        private static int childCount = 0;

        /// <summary>
        /// Wait for all processes to finish
        /// </summary>
        private static ManualResetEventSlim eventSlim = new ManualResetEventSlim();

        /// <summary>
        /// Chain native commands.
        /// e.g. copy-async "C:\WINDOWS\system32\ipconfig.exe" "C:\Program Files (x86)\Microsoft VS Code\bin\cat.exe"
        /// </summary>
        /// <remarks>
        /// Expect executables and their arguments to be represented in this way:
        ///   [executable]|[arguments]
        /// </remarks>
        public static void Main(string[] args)
        {
            if (args.Length == 0)
            {
                // Use default command chain
                if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
                {
                    Console.WriteLine("Demo piping behavior: 'ipconfig.exe | cat.exe'");
                    args = new string[] { @"C:\WINDOWS\system32\ipconfig.exe",
                                          @"C:\Program Files (x86)\Microsoft VS Code\bin\cat.exe" };
                }
                else if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux))
                {
                    Console.WriteLine("Demo pipeing behavior: 'ifconfig | cat'");
                    args = new string[] { "/sbin/ifconfig",
                                          "/bin/cat" };
                }
            }

            List<Process> processes = new List<Process>();
            for (int i = 0; i < args.Length; i++)
            {
                ProcessStartInfo startInfo = null;
                // Expect executables and their arguments to be represented in this way: [executable]|[arguments]
                int colonIndex = args[i].IndexOf('|');
                if (colonIndex == -1 && !string.IsNullOrWhiteSpace(args[i]))
                {
                    startInfo = new ProcessStartInfo(args[i]);
                }
                else if (colonIndex > 0)
                {
                    startInfo = new ProcessStartInfo(
                                    args[i].Substring(0, colonIndex),
                                    args[i].Substring(colonIndex + 1));
                }
                else
                {
                    throw new ArgumentException("args are incorrect.");
                }

                if (i > 0) { startInfo.RedirectStandardInput = true; }
                if (i < args.Length - 1) { startInfo.RedirectStandardOutput = true; }

                Process proc = new Process() { StartInfo = startInfo, EnableRaisingEvents = true };
                proc.Exited += new EventHandler(ProcessExitHandler);

                processes.Add(proc);
            }

            childCount = processes.Count;
            List<ProcessPipe> pipes = new List<ProcessPipe>();

            Process upstream = null;
            for (int i = 0; i < processes.Count; i++)
            {
                Process proc = processes[i];
                proc.Start();

                if (upstream != null)
                {
                    var pipe = new ProcessPipe(upstream, proc);
                    pipe.Start();
                    pipes.Add(pipe);
                }
                upstream = proc;
            }

            foreach (var pipe in pipes)
            {
                pipe.Wait();
            }

            // Wait for all child process to finish
            eventSlim.Wait();
        }

        internal static void ProcessExitHandler(object sender, System.EventArgs e)
        {
            // Set the event if it's the last child process
            if (Interlocked.Decrement(ref childCount) == 0)
                eventSlim.Set();
        }
    }

    internal class ProcessPipe
    {
        private byte[] _buffer;
        private Process _upstream, _downstream;
        private Thread _thread;

        internal ProcessPipe(Process upstream, Process downstream)
        {
            _buffer = new byte[1024];
            _upstream = upstream;
            _downstream = downstream;
        }

        private void StartPiping()
        {
            int count = 0;
            try {
                while ((count = _upstream.StandardOutput.BaseStream.Read(_buffer, 0, 1024)) > 0)
                {
                    _downstream.StandardInput.BaseStream.Write(_buffer, 0, count);
                    _downstream.StandardInput.BaseStream.Flush();
                }
            }
            catch (Exception ex)
            {
                Console.WriteLine("Exception when reading/writing from worker-thread: {0}", ex.Message);
            }
            finally
            {
                Console.WriteLine("{0} exiting, disposing StandardInput of process '{1} {2}'", _thread.Name, _downstream.StartInfo.FileName, _downstream.StartInfo.Arguments);
                try {
                    _downstream.StandardInput.Dispose();
                } catch (Exception ex) {
                    Console.WriteLine("Disposeing StandardInput failed: {0}", ex.Message);
                }
            }
        }

        internal void Start()
        {
            _thread = new Thread(new ThreadStart(StartPiping));
            _thread.Name = string.Format("{0} -> {1} :Piping Thread", _upstream.StartInfo.FileName, _downstream.StartInfo.FileName);
            _thread.Start();
        }

        internal void Wait()
        {
            _thread.Join();
        }
    }
}

slow-outputer: slow.exe

using System;
using System.Threading;

namespace NativePipe
{
    public class Slow
    {
        private static string[] outputs = new string[] {
            "This is a console app to write output in slow manner",
            "Hello world",
            "C# across platforms is awesome!",
            "We cannot go back time",
            "Unless we have a time machine~",
            "One last output :)"
        };

        public static void Main(string[] args)
        {

            foreach (string str in outputs)
            {
                Console.WriteLine(str);
                Console.Out.Flush();
                Thread.Sleep(2000);
            }
        }
    }
}

mimic cat.exe with logging: mycat.exe

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Runtime.InteropServices;
using System.Text;
using System.Threading;
using System.Threading.Tasks;

namespace ConsoleApplication
{
    public class Program
    {
        public static void Main(string[] args)
        {
            if (args.Length != 1)
            {
                Console.Error.WriteLine("Wrong Usage!");
                return;
            }

            bool first = true;
            string fileName = args[0];
            string envVarName = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "USERPROFILE" : "HOME";
            string homeDir = Environment.GetEnvironmentVariable(envVarName);
            string tempDir = Path.Combine(homeDir, "temp");
            if (!Directory.Exists(tempDir)) { Directory.CreateDirectory(tempDir); }

            using (FileStream fs = new FileStream(Path.Combine(tempDir, fileName), FileMode.Create))
            using (StreamWriter sw = new StreamWriter(fs, Encoding.UTF8){AutoFlush = true})
            {
                sw.WriteLine("IsInputRedirected? {0}", Console.IsInputRedirected);
                sw.WriteLine("IsOutputRedirected? {0}", Console.IsOutputRedirected);
                sw.WriteLine("IsErrorRedirected? {0}", Console.IsErrorRedirected);

                while (true)
                {
                    if (first) {
                        first = false;
                        sw.WriteLine("-- About to read from stdin --");
                    }
                    string line = Console.ReadLine(); // <== Block here in Linux even though StandardInput of the process has been disposed
                    if (line == null)
                    {
                        sw.WriteLine("Console.In closed.");
                        break;
                    }
                    else
                    {
                        sw.WriteLine(line);
                        Console.WriteLine(line);
                        Console.Out.Flush();
                        Thread.Sleep(1000);
                    }
                }
            }
        }
    }
}

project.json

{
    "name": <executable-name>, 
    "version": "1.0.0-*",

    "buildOptions": {
        "debugType": "portable",
        "emitEntryPoint": true
    },

    "frameworks": {
        "netcoreapp1.1": {
            "imports": [ "dnxcore50" ],
            "dependencies": {
                "Microsoft.NETCore.App": "1.1.0-preview1-001100-00"
            }
        }
    },

    "runtimes": {
        "win10-x64": { },
        "ubuntu.14.04-x64": { }
    }
}

Behavior on Windows

Chaining 3 processes runs successfully:

## First add path to slow.exe and mycat.exe to environment variable PATH
E:\arena\dotnetApp\pipe-threading\bin\Debug\netcoreapp1.1\win10-x64>work-thread.exe slow "mycat|cat1" "mycat|cat2"
This is a console app to write output in slow manner
Hello world
C# across platforms is awesome!
We cannot go back time
Unless we have a time machine~
One last output :)
slow -> mycat :Piping Thread exiting, disposing StandardInput of process 'mycat cat1'
mycat -> mycat :Piping Thread exiting, disposing StandardInput of process 'mycat cat2'

Logging from mycat.exe shows that Console.ReadLine() == null was hit in both mycat.exe processes (note the line Console.In closed. in the logs):
cat1

IsInputRedirected? True
IsOutputRedirected? True
IsErrorRedirected? False
-- About to read from stdin --
This is a console app to write output in slow manner
Hello world
C# across platforms is awesome!
We cannot go back time
Unless we have a time machine~
One last output :)
Console.In closed.

cat2

IsInputRedirected? True
IsOutputRedirected? False
IsErrorRedirected? False
-- About to read from stdin --
This is a console app to write output in slow manner
Hello world
C# across platforms is awesome!
We cannot go back time
Unless we have a time machine~
One last output :)
Console.In closed.

Behavior on Linux

Chaining the same 3 processes hangs:

## First add path to slow and mycat to environment variable PATH
user@machine:~/arena/pipe-threading/bin/Debug/netcoreapp1.1/ubuntu.14.04-x64$ ./work-thread slow "mycat|cat1" "mycat|cat2"
This is a console app to write output in slow manner
Hello world
C# across platforms is awesome!
We cannot go back time
Unless we have a time machine~
One last output :)
slow -> mycat :Piping Thread exiting, disposing StandardInput of process 'mycat cat1'
<hangs here>

Logging from mycat shows that Console.ReadLine() == null was NOT hit in both mycat processes (note the line Console.In closed. is missing in the logs):
cat1

user@machine:~/temp$ cat cat1
IsInputRedirected? True
IsOutputRedirected? True
IsErrorRedirected? False
-- About to read from stdin --
This is a console app to write output in slow manner
Hello world
C# across platforms is awesome!
We cannot go back time
Unless we have a time machine~
One last output :)

cat2

user@machine:~/temp$ cat cat2
IsInputRedirected? True
IsOutputRedirected? False
IsErrorRedirected? False
-- About to read from stdin --
This is a console app to write output in slow manner
Hello world
C# across platforms is awesome!
We cannot go back time
Unless we have a time machine~
One last output :)

[vors]

Thank you, @daxian-dbw !
@karelz for the context, we are working on PowerShell/PowerShell#1908

In PowerShell, if we piping native | native we would like to avoid creating PowerShell pipeline at all and just do the stdin -> stdout redirection.

[ianhays]

Console.ReadLine() is blocked in Linux even though the StandardInput of the process has been disposed

All Console.ReadLine does is call the ReadLine method of Console.In. It looks like your problem is actually that _downstream.StandardInput.Dispose(); is not what doing what you want it to on Unix.

Here's my thinking. On Windows, stdin is a handle that is unique to a process and when you call _downstream.StandardInput.Dispose(); you're disposing the StreamWriter around the FileStream that was opened with that handle. This closes the handle, so in the downstream process, attempts to read the Console.In that wraps that handle don't return anything.

On Unix, stdin is a file descriptor. When we open the StandardInput StreamWriter in Process, it's via a FileStream wrapped around a SafeFileHandle object.

Here's where the difference is: we don't destroy that file descriptor when the SafeFileHandle is disposed (when the filestream is closed which happens when Process.StandardInput is disposed). This means that the fd is still available to the downstream process, so the downstream ReadLine blocks for data on the still-open fd.

From poking around that's my understanding. but @stephentoub can correct me here if I'm missing something.

If that's not it, then the next place I would look for a potential issue is in ConsolePal.Unix where we create/cache/get the Input stream(reader).

[vors]

It looks like your problem is actually that _downstream.StandardInput.Dispose(); is not what doing what you want it to on Unix.

I thought that the whole point of being a cross-platform framework is to abstract away such things and allow people to write code that predictable runs the same way on all platforms.

Marking issue with label enhancement also suggests that you don't think there is a bug in the existing behavior. I understand the reasoning about the possible source of the problem and I think it doesn't justify the existing inconsistency of the whole scenario.

[ianhays]

Marking issue with label enhancement also suggests that you don't think there is a bug in the existing behavior.

The label is not an end-all thing. It's just a quick and easy way to triage issues. After discussion and more in-depth triage, we refine the labels once we know more about the issue. I marked it as 'enhancement' because I'm not yet certain that it's a bug and not just a platform inconsistency that is unavoidable (but possible mitigable through some enhancement). The label has no affect on how we prioritize issues - it's there purely for organization.

I thought that the whole point of being a cross-platform framework is to abstract away such things and allow people to write code that predictable runs the same way on all platforms.
I understand the reasoning about the possible source of the problem and I think it doesn't justify the existing inconsistency of the whole scenario.

Note that I didn't say the behavior was intended or even correct; I merely offered a possible explanation for why it behaves as it does today to stir up discussion and possible solutions.

[daxian-dbw]

@ianhays thanks for looking into this! I feel it's more like a bug because the issue happens only if I chain more than 2 processes on Linux, but not when only 2 processes are chained in this way.

When only 2 processes are chained by ProcessPipe, Console.ReadLine in the downstream process will correctly read null, and that means _downstream.StandardInput.Dispose() actually takes effect. As for why it doesn't work when more than 2 processes are chained is beyond my understanding.

[ianhays]

Feel free to investigate and push a PR referencing this issue. Marking as bug and up for grabs.

[vors]

I believe it's a root cause of PowerShell/PowerShell#3321

Here is a smaller repro

using System.Diagnostics;

namespace R15 {

    public class Repro
    {
        public static void Register(Process process)
        {
            process.OutputDataReceived += new DataReceivedEventHandler(OutputHandler);
            process.BeginOutputReadLine();
        }

        private static void OutputHandler(object sender, DataReceivedEventArgs outputReceived)
        {
            if (outputReceived.Data != null)
            {
                System.Console.WriteLine("NON NULL " + outputReceived.Data);
            }
            else
            {
                System.Console.WriteLine("NULL");
            }
        }

        public static Process GetProcess()
        {
            var info = new ProcessStartInfo();
            info.FileName = "grep";
            info.Arguments = "1";
            info.RedirectStandardInput = true;
            info.RedirectStandardOutput = true;
            
            var process = new Process();
            process.StartInfo = info;
            process.Start();

            return process;
        }

        public static void Main(string[] args)
        {
            var p1 = GetProcess();
            var p2 = GetProcess(); // if you comment out this line, or uncomment p2.StandardInput.Dispose(); everything works as expected
            
            Register(p1);

            p1.StandardInput.WriteLine("1");
            p1.StandardInput.Dispose();

            //p2.StandardInput.Dispose();
        }
    }
}

If you run it on Unix, the correct behavior would be to get output out of OutputHandler.
Currently it doesn't produce any output.
It works if:

• you have only 1 process, not 2
• if you call Dispose on both of them

Note that in my example processes are not chained.

[daxian-dbw]

I now have a smaller repro, and it turns out this happens as long as more than one processes have StandardInput redirected in an application.

Take the repro below as an example, when I start proc2 and proc3 and redirect their StandardInput, Console.ReadLine() in proc2 continues to block after proc2.StandardInput.Close() is called. However, if I don't start proc3, then Console.ReadLine() in proc2 returns null as expected.

I think the root cause may lie in SafeFileHandle that wraps a pipe file descriptor or in FileStream that is on top of a pipe file descriptor. I'm currently trying to debug FileStream in System.Private.CoreLib.dll to find out more. But @ianhays and @stephentoub if you can point me to the right direction, that would be very helpful.

using System.Diagnostics;

namespace Simple {

    public class Repro
    {
        public static void Main(string[] args)
        {
            var startInfo2 = new ProcessStartInfo("mycat", "/home/dongbo/temp/catN");
            startInfo2.RedirectStandardInput = true;
            var proc2 = new Process() { StartInfo = startInfo2 };

            var startInfo3 = new ProcessStartInfo("mycat", "/home/dongbo/temp/catP");
            startInfo3.RedirectStandardInput = true;
            var proc3 = new Process() { StartInfo = startInfo3 };

            // Start two processes and redirect StandardInput for both of them
            proc2.Start();
            proc3.Start();

            for (int i = 0; i < 1; i ++)
            {
                string output = $"Output {i}";
                proc2.StandardInput.WriteLine(output);
            }
            proc2.StandardInput.Close(); // <---- Close the standard input stream, but `Console.ReadLine()` in proc2 continues to block.

            proc2.WaitForExit();
            //s_proc3.WaitForExit();
        }
    }
}

[daxian-dbw]

OK, I think I nailed it. The root cause seems to be in Interop.Sys.ForkAndExecProcess. When creating the first child process, the parent process holds a file descriptor to the write end of the redirected stdin pipe. Then when creating the second child process, the file descriptor is inherited by the second child process, and thus there are now 2 file descriptors referencing the same write end of the pipe. Therefore, when proc2.StandardInput.Close() is called, the first descriptor held by the parent process is released, but the inherited one by the second child process is still open and thus the read end of the stdin pipe doesn't receive EOF.
I will try to come up with a PR.

[daxian-dbw]

Looks like this issue falls in the bucket of #13943.
@ianhays and @stephentoub could you please advise if it's OK to deal with the pipe file descriptors created for stdin, stdout and stderr redirection specially -- make the file descriptors held by the parent process not inheritable by subsequent child processes by using fcntl64(fd, F_SETFD, FD_CLOEXEC)?

[stephentoub]

@daxian-dbw, thanks for investigating, and nice job finding the root cause. I think that solution makes sense. There will still be a tiny window (between the call to pipe and the call to fcntl) where a concurrently started process could inherit the handle, but that should be very rare. As for the actual fix, I'd suggest adding your own function that just does the combination of pipe and fcntl, and then substitute in that call for the ones made here:
https://github.com/dotnet/corefx/blob/473463e644cba090257b6fde42a17af5bc01b01b/src/Native/Unix/System.Native/pal_process.cpp#L140-L141
You'll then also need to remove the close calls made here:
https://github.com/dotnet/corefx/blob/473463e644cba090257b6fde42a17af5bc01b01b/src/Native/Unix/System.Native/pal_process.cpp#L166-L169
as the child process won't have inherited those.

[stephentoub]

Also, @daxian-dbw, can we assume this is impacting PowerShell? (It looks like that's the case from some of the linked issues.)

[daxian-dbw]

@stephentoub thanks for the suggestion. Yes, this is impacting PowerShell. We need to support pipeline between native commands, and that work is blocked by this issue.

I found the function SystemNative_Pipe(int32_t pipeFds[2], int32_t flags) and it's used in here to replace a call to pipe. I think I probably should use the same function for this fix.
And I think I should also remove the close calls made here: https://github.com/dotnet/corefx/blob/473463e644cba090257b6fde42a17af5bc01b01b/src/Native/Unix/System.Native/pal_process.cpp#L179-L181, as the descriptors will be closed on execve call in the child process.

[stephentoub]

I think I probably should use the same function for this fix.

No, you can't. Specifying CLOEXEC to that will close both file descriptors, both ends of the pipe. You don't want to do that; it'll break the whole purpose of this. You only want to specify CLOEXEC on the parent process' side of the pipe, not on the side of the pipe that must be inherited into the child.

And I think I should also remove the close calls made here

No, you don't want to remove that. The child side of the pipe needs to be inherited into the child, otherwise it has no way to communicate with the parent.

[daxian-dbw]

@stephentoub The file descriptors of the pipe ends that are going to be used by the child process are dup'ed at https://github.com/dotnet/corefx/blob/473463e644cba090257b6fde42a17af5bc01b01b/src/Native/Unix/System.Native/pal_process.cpp#L173-L175, so it seems OK to have the old pipe file descriptors closed on the call to execve in the child process -- the pipes are still open, because the dup'ed file descriptors in the child process (STDIN_FILENO, STDOUT_FILENO and STDERR_FILENO) are still referencing the pipes.

I tested my changes with SystemNative_Pipe and it seems working fine. Can you please take another look to see if my understanding is correct?

[stephentoub]

The file descriptors of the pipe ends that are going to be used by the child process are dup'ed at

That's already in the child process.

[daxian-dbw]

Yes, but it's fine as long as they are closed on the call to execve, right?

[stephentoub]

Yes, but it's fine as long as they are closed on the call to execve, right?

Oh, I see, I misunderstood what you were suggesting. Yes, that should be fine then.

[daxian-dbw]

Thanks @stephentoub, I will submit a PR and cc you. How should I proceed with the test? I assume I need to submit functional tests along with product code changes.

[stephentoub]

How should I proceed with the test? I assume I need to submit functional tests along with product code changes.

Yes, please. I expect it should be pretty easy for you to come up with a short [Fact] that would hang without the fix and quickly succeed with?

[daxian-dbw]

I will take a look at the existing process tests and write one for this fix. Thanks!

[stephentoub]

Thank you!

[stephentoub]

dotnet/corefx#19997 was merged to fix this in master, but leaving this issue open to track a release/2.0 port once approved.

[daxian-dbw]

@stephentoub Thank you so much for getting the fix approved for netcoreapp2.0!

[daxian-dbw]

Close the issue as the fix has been ported to release/2.0 via dotnet/corefx#20001

[paultetley]

Apologies in advance is this is wrong forum, but is there any chance of this fix being released in the 1.0 stream? As I understand it, it's a long wait until net core 2.0 and the workarounds we've found for this issue aren't pretty.

[stephentoub]

it's a long wait until net core 2.0

The schedule for 2.0 is outlined here: https://github.com/dotnet/core/blob/master/roadmap.md,

is there any chance of this fix being released in the 1.0 stream?

@leecow, what's the approach for the community requesting fixes be ported back to 1.x releases?

[karelz]

@paultetley what is acceptable wait time for you?
Which apps/projects does it block for you? (are you using PowerShell, or something else?)

[leecow]

@stephentoub, @paultetley - Tagging the issue for the 1.0.x or 1.1.x milestones will put it on the triage list for consideration. Including detailed impact of the issue for developer and end customer is very helpful during triage.

[stephentoub]

@leecow, sorry to be dense, but tag it how? You mean change the Milestone back to be 1.0 or 1.1 instead of 2.0?

[karelz]

We should not tag 1.x (milestone set) any fixed issue as this one.
We may need to file a new "porting" issue where we can discuss details. I'd suggest first to wait on info from @paultetley, then we can decide if it is worth creating new issue for further discussion/tracking or not.

[leecow]

@stephentoub - sorry I was vague. If this issue (#13447) is going to be fixed in 2.0 then we'll need another issue opened to track the triage and possible port to the 1.0 / 1.1.

[paultetley]

Hi all, Thanks for your responses on this. We recently ported a production app from Mono on Linux to .Net Core (on Linux) to fix a problem we had with the Mono GC doing huge STW pauses under heavy IO load. The Core runtime GC could be shown not to exhibit the fault and we also liked the idea that this was platform with a big future. Our application is a webapp that manages a lot to system resources and attached hardware on the server, and it frequently creates processes to do this, some of them long running. This will not be one of your more common use cases but we chose C# on Linux because it is a good general purpose language and at first look seemed sufficiently mature. In retrospect, we may have jumped in too soon, not just because of this issue, but also one with file descriptors and child processes which is already linked above. We have a work-around for this bug, however I think anyone using System.Diagnostics.Process in anything but the most trivial way is going to be bitten, and since you have a fix, it seems worth including in a point release. Regards Paul Tetley

[karelz]

If there is workaround, nobody is blocked, then it is cost-ineffective to port anything to 1.0. The right thing is to wait until 2.0, which is honestly not that far away.

Any change in servicing branch has to have some non-trivial value, because it costs work and it introduces risk (10% of all changes introduce unexpected side-effects, no matter how much testing or code reviews you do).

Moreover, if we would backport every important bug fix, we would end up backporting 10% of 2.0 into 1.0, which doesn't make sense -- more bug fixes and features is what new version (2.0 in this case) is about. Servicing releases should be left for high-impactful and/or hard-to-workaround problems. Neither of those conditions is met in this case.