dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.05k stars 4.69k forks source link

Running a System.Diagnostics.Process with both async stdout/stderr redirection blocks a ThreadPool thread #81896

Open drauch opened 1 year ago

drauch commented 1 year ago

Description

If you run a System.Diagnostics.Process and redirect both stdout and stderr in an async way (using the BeginOutputReadLine/BeginErrorReadLine methods) it blocks a thread pool thread.

Reproduction Steps

The following C# interactive notebook:

using System.Diagnostics;
using System.Threading;

bool stop = false;
var threadPoolMonitoringThread = new Thread(() => { while (!stop) { Console.WriteLine($"{DateTime.Now}: ThreadCount: {ThreadPool.ThreadCount}, PendingWorkItemCount: {ThreadPool.PendingWorkItemCount}"); Thread.Sleep(1000); } });
threadPoolMonitoringThread.Start();

var tasks = Enumerable.Range(0, 10).Select(async _ => {
  using (Process process = new Process())
    {
      process.StartInfo.FileName = "pwsh.exe";
      process.StartInfo.Arguments = "-Command Start-Sleep -Seconds 10";
      process.StartInfo.UseShellExecute = false;
      process.StartInfo.CreateNoWindow = true;
      process.StartInfo.RedirectStandardOutput = true;
      process.StartInfo.RedirectStandardError = true;

      process.OutputDataReceived += (s, a) => { };
      process.ErrorDataReceived += (s, a) => { };

      process.Start();    

      process.BeginOutputReadLine();      
      process.BeginErrorReadLine();

      await process.WaitForExitAsync();      
    }
});

await Task.WhenAll(tasks);
stop = true;

reproduces the problem. You can immediately see the PendingWorkItemCount raising to very high numbers, while actually all the WaitForExitAsync calls should not block threads on the thread pool at all.

If you either remove the BeginOutputReadLine or the BeginErrorReadLine call the PendingWorkItemCount stays 0 as expected. So it must have something to do with redirecting both streams.

Expected behavior

The thread pool should not be filled by 10 parallel async calls.

Actual behavior

The thread pool is filled up, because each call blocks a thread pool thread.

Regression?

No response

Known Workarounds

No response

Configuration

Reproduced on .NET 6 and .NET 7

Other information

I'm pretty confident that I'm using the Process API correctly, also various other Process-class-Wrappers like MedallionShell or CliWrap are having the same problem.

ghost commented 1 year ago

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process See info in area-owners.md if you want to be subscribed.

Issue Details
### Description If you run a System.Diagnostics.Process and redirect both stdout and stderr in an async way (using the BeginOutputReadLine/BeginErrorReadLine methods) blocks a thread pool thread. ### Reproduction Steps The following C# interactive notebook: ``` using System.Diagnostics; using System.Threading; bool stop = false; var threadPoolMonitoringThread = new Thread(() => { while (!stop) { Console.WriteLine($"{DateTime.Now}: ThreadCount: {ThreadPool.ThreadCount}, PendingWorkItemCount: {ThreadPool.PendingWorkItemCount}"); Thread.Sleep(1000); } }); threadPoolMonitoringThread.Start(); var tasks = Enumerable.Range(0, 10).Select(async _ => { using (Process process = new Process()) { process.StartInfo.FileName = "pwsh.exe"; process.StartInfo.Arguments = "-Command Start-Sleep -Seconds 10"; process.StartInfo.UseShellExecute = false; process.StartInfo.CreateNoWindow = true; process.StartInfo.RedirectStandardOutput = true; process.StartInfo.RedirectStandardError = true; process.OutputDataReceived += (s, a) => { }; process.ErrorDataReceived += (s, a) => { }; process.Start(); process.BeginOutputReadLine(); process.BeginErrorReadLine(); await process.WaitForExitAsync(); } }); await Task.WhenAll(tasks); stop = true; ``` reproduces the problem. You can immediately see the PendingWorkItemCount raising to very high numbers, while actually all the WaitForExitAsync blocks should not block threads on the thread pool at all. If you either remove the `BeginOutputReadLine` or the `BeginErrorReadLine` call the PendingWorkItemCount stays 0 as expected. ### Expected behavior The thread pool should not be filled by 10 parallel async calls. ### Actual behavior The thread pool is filled up, because each call blocks a thread pool thread. ### Regression? _No response_ ### Known Workarounds _No response_ ### Configuration Reproduced on .NET 6 and .NET 7 ### Other information I'm pretty confident that I'm using the Process API correctly, also various other Process-class-Wrappers like MedallionShell are having the same problem.
Author: drauch
Assignees: -
Labels: `area-System.Diagnostics.Process`
Milestone: -
drauch commented 1 year ago

Here is a Dropbox link to the Polyglot Notebook I've posted above: https://www.dropbox.com/s/qjnuwmv7zu1306q/Repro81896.ipynb?dl=0

stephentoub commented 1 year ago

If you look in the Parallel Stacks window in VS, you'll see something like:

image

That's because Process is creating anonymous pipes for the child process's stdout/stderr, and anonymous pipes on Windows don't support overlapped I/O. As such, all reads are blocking.

drauch commented 1 year ago

Thank you for your quick reply.

That's because Process is creating anonymous pipes for the child process's stdout/stderr, and anonymous pipes on Windows don't support overlapped I/O. As such, all reads are blocking.

I understand, is there a suitable workaround? We want to run a process as part of an ASP.NET Core request and blocking thread pool threads is a no-go in that area.

Requirements:

Would really appreciate it :-)

Best regards, D.R.

stephentoub commented 1 year ago

is there a suitable workaround?

My expectation is that you wouldn't hit such an issue on Linux, so if deploying on Linux is an option, you could try that.

Other than that, I can't think of any workarounds.

Someone could experiment with using something other than anonymous pipes, e.g. maybe named pipes could be used instead (e.g. with the server pipe created to expect overlapped I/O). I don't know what the ramifications of that would be for launched processes, though, and whether such a difference would be observable in an impactful way.

drauch commented 1 year ago

We're stuck on Windows unfortunately. Thanks for trying anyways.

fschmied commented 1 year ago

So, if we want to avoid ThreadPool threads from being blocked, the best thing we can do is to avoid the Begin... methods and instead use something like this, right?

var t1 = Task.Factory.StartNew (() => process.StandardOutput.ReadToEnd(), TaskCreationOptions.LongRunning);
var t2 = Task.Factory.StartNew (() => process.StandardError.ReadToEnd(), TaskCreationOptions.LongRunning);
var t3 = process.WaitForExitAsync();

await Task.WhenAll(t1, t2, t3);
fschmied commented 1 year ago

Should this be documented on the Process class docs? (https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.process?view=net-7.0)

stephentoub commented 1 year ago

the best thing we can do is to avoid the Begin... methods and instead use something like this, right?

To my knowledge that's currently the way you'd avoid blocking pool threads.

davidfowl commented 1 year ago

I understand, is there a suitable workaround? We want to run a process as part of an ASP.NET Core request and blocking thread pool threads is a no-go in that area.

Is this a remote console via the browser (using websockets to marshal the std/in/out/err)?

drauch commented 1 year ago

I understand, is there a suitable workaround? We want to run a process as part of an ASP.NET Core request and blocking thread pool threads is a no-go in that area.

Is this a remote console via the browser (using websockets to marshal the std/in/out/err)?

No, requests trigger the execution of various different Windows executables. The output is used to return suitable responses to the clients. Unfortunately, we can't change the executables either (i.e., to read the output from, e.g., a file).

davidfowl commented 1 year ago

I see, my only advice would be to limit concurrency of those requests or use a separate pool of threads.

fbrosseau commented 1 year ago

is there a suitable workaround?

My expectation is that you wouldn't hit such an issue on Linux, so if deploying on Linux is an option, you could try that.

Other than that, I can't think of any workarounds.

Someone could experiment with using something other than anonymous pipes, e.g. maybe named pipes could be used instead (e.g. with the client pipe created to expect overlapped I/O). I don't know what the ramifications of that would be for launched processes, though, and whether such a difference would be observable in an impactful way.

Regular pipes would work, the host process creates both the server (overlapped) and the client (non-overlapped), and client end goes to the child process. The client pipe must remain non-overlapped because 99% of arbitrary processes are not ready to receive an overlapped handle for their stdout/stderr (although nothing would prevent it from working if the child process were overlapped-aware).

Short of doing a scheme like this within the framework, the only scalable way to have very many child processes is to pinvoke CreateProcess&co yourself and do exactly this, and that's not the most trivial.

stephentoub commented 1 year ago

Regular pipes would work

By "regular", you mean "named", like I stated, yes?

fbrosseau commented 1 year ago

Yes. Effectively "anonymous named pipes", named pipes with random names.

stephentoub commented 1 year ago

Right.

chylex commented 1 year ago

This really should be documented... I just spent several hours hunting for why my application became almost entirely unresponsive for several minutes, and the majority of the worker threads were inexplicably stuck in NtReadFile with no obvious reason why. PerfView and dotnet-dump were not of much help.

obrazek

If the documentation states:

Begins asynchronous read operations on the redirected StandardOutput stream of the application.

then I have no reason to suspect these methods to be the reason why the entire worker pool stops running tasks.