-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Console.ReadLine() is blocked in Linux even though the StandardInput of the process has been disposed #19277
Comments
Thank you, @daxian-dbw ! In PowerShell, if we piping |
All Console.ReadLine does is call the ReadLine method of Console.In. It looks like your problem is actually that Here's my thinking. On Windows, stdin is a handle that is unique to a process and when you call 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 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). |
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 |
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.
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. |
@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 |
Feel free to investigate and push a PR referencing this issue. Marking as |
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
Note that in my example processes are not chained. |
I now have a smaller repro, and it turns out this happens as long as more than one processes have Take the repro below as an example, when I start I think the root cause may lie in
|
OK, I think I nailed it. The root cause seems to be in |
Looks like this issue falls in the bucket of #13943. |
@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: |
Also, @daxian-dbw, can we assume this is impacting PowerShell? (It looks like that's the case from some of the linked issues.) |
@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 |
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.
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. |
@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 I tested my changes with |
That's already in the child process. |
Yes, but it's fine as long as they are closed on the call to |
Oh, I see, I misunderstood what you were suggesting. Yes, that should be fine then. |
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. |
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? |
I will take a look at the existing process tests and write one for this fix. Thanks! |
Thank you! |
dotnet/corefx#19997 was merged to fix this in master, but leaving this issue open to track a release/2.0 port once approved. |
@stephentoub Thank you so much for getting the fix approved for |
Close the issue as the fix has been ported to |
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. |
The schedule for 2.0 is outlined here: https://github.com/dotnet/core/blob/master/roadmap.md,
@leecow, what's the approach for the community requesting fixes be ported back to 1.x releases? |
@paultetley what is acceptable wait time for you? |
@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. |
@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? |
We should not tag 1.x (milestone set) any fixed issue as this one. |
@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. |
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
|
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. |
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
return0
, the worker thread dispose theStandardInput
of the downstream process, and in that case,Console.ReadLine()
in the downstream process is supposed to returnnull
, 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 theStandardInput
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
slow-outputer: slow.exe
mimic cat.exe with logging: mycat.exe
project.json
Behavior on Windows
Chaining 3 processes runs successfully:
Logging from
mycat.exe
shows thatConsole.ReadLine() == null
was hit in bothmycat.exe
processes (note the lineConsole.In closed.
in the logs):cat1
cat2
Behavior on Linux
Chaining the same 3 processes hangs:
Logging from
mycat
shows thatConsole.ReadLine() == null
was NOT hit in bothmycat
processes (note the lineConsole.In closed.
is missing in the logs):cat1
cat2
The text was updated successfully, but these errors were encountered: