Tyrrrz / CliWrap

Library for running command-line processes
MIT License
4.36k stars 268 forks source link

PipeTarget.ToDelegate blocks forever in StreamReader #185

Closed vivainio closed 1 year ago

vivainio commented 1 year ago

Version

ver 3.5.0

Details

This is a spin off issue from: https://github.com/Tyrrrz/CliWrap/issues/85

Running a particular process ("cmd /c mybatfile.cmd") never finishes.

I have pinpointed this to this StreamReader reading in PipeTarget.cs:

    /// <summary>
    /// Creates a pipe target that invokes the specified asynchronous delegate on every line written.
    /// </summary>
    public static PipeTarget ToDelegate(Func<string, Task> handleLineAsync, Encoding encoding) =>
        Create(async (origin, cancellationToken) =>
        {
            using var reader = new StreamReader(origin, encoding, false, BufferSizes.StreamReader, true);
            await foreach (var line in reader.ReadAllLinesAsync(cancellationToken).ConfigureAwait(false))
                await handleLineAsync(line).ConfigureAwait(false);
        });

Looks like this foreach loop never completes with this particular process.

I have a reliable reproduction, but I can't share the target process. For most other processes, this works correctly.

Steps to reproduce

  await foreach (var cmdEvent in cmd.ListenAsync())
            {
                switch (cmdEvent)
                {
                    case StartedCommandEvent start:
                        write(v, cname, "Start pid " + start.ProcessId);
                        RunningPid[index] = start.ProcessId;
                        break;
                    case StandardOutputCommandEvent stdOut:
                        write(v, cname, stdOut.Text); 
                        break;
                    case StandardErrorCommandEvent stdErr:
                        write(v, "red", stdErr.Text); 
                        break;
                    case ExitedCommandEvent ex:
                        return (ex.ExitCode, 0);
                    default:
                        write("UNK", "red", "Unknown event :" + cmdEvent.ToString());
                        break;
                }
            }
vivainio commented 1 year ago

I have a reliable reproduction available, but can't share it (proprietary code). If you try writing the logic using something apart from this StreamReader, I can test it from a branch.

Tyrrrz commented 1 year ago

Does running the script directly as mybatfile.cmd instead of via cmd have any effect?

vivainio commented 1 year ago

Does running the script directly as mybatfile.cmd instead of via cmd have any effect?

No.

BUT I think I have a reproducible scenario.

Create a cmd file with this content:

start /min python

This starts python.exe in the background, but the cmd file exits. CliWrap gets the exited event but gets blocked forever in reading the outputs. My expectation here is that CliWrap should complete the process as well, regardless of the status of the streams.

Tyrrrz commented 1 year ago

Does running the script directly as mybatfile.cmd instead of via cmd have any effect?

No.

BUT I think I have a reproducible scenario.

Create a cmd file with this content:

start /min python

This starts python.exe in the background, but the cmd file exits. CliWrap gets the exited event but gets blocked forever in reading the outputs. My expectation here is that CliWrap should complete the process as well, regardless of the status of the streams.

Ok, thanks for the repro. I'll try to debug it after the holidays.

Tyrrrz commented 1 year ago

Hi @vivainio. I tried reproducing it with the following setup.

start /min python
var cmd = Cli.Wrap("test.cmd");

await foreach (var e in cmd.ListenAsync())
{
    Console.WriteLine(e);
}

Upon running the code, I was able to receive StartedCommandEvent (with process ID), a couple of StandardOutputCommandEvent and then the control halted. Once I closed the spawned python window, I got the final ExitedCommandEvent (with exit code) and the execution of my code terminated.

Note that I was able to observe the exact same behavior with ExecuteBufferedAsync() instead of ListenAsync(), so I don't think this is specific to a particular execution model.

I was able to avoid the blocking behavior by changing the script to this:

start /B /min python

The /B switch, according to documentation, is used to "start application without creating a new window". Not sure exactly how that affects this scenario though.

Finally, I was also able to reproduce the blocking behavior (without the /B switch) using barebones Process class with the following minimal code:

using var process = new Process
{
    StartInfo =
    {
        FileName = "test.cmd",
        RedirectStandardOutput = true
    }
};

process.Start();

var stdout = await process.StandardOutput.ReadToEndAsync();

await process.WaitForExitAsync();

Note that the above code only blocks if the standard output is redirected and read as shown above.

Based on this, I can draw the conclusion that the spawned process somehow retains the standard output/error/input handles of the parent process. Which is what's preventing CliWrap (and also Process in the above example) from yielding control when the process exits.

Also note that it's normal that a process exits before its standard streams are closed, this is not a special case in itself.

I'm not sure what can be done in this scenario, as it seems like one of many quirks that Windows has with console processes. Is it viable for you to amend the script to use the /B switch (or something similar)?

vivainio commented 1 year ago

@Tyrrrz I can't use /B switch as I need the output console.

Since the underlying Process fires Exited event, which is never seen by CliWrap because of forever blocking output streams, I think a good solution is exposing the Exited even in CliWrap.

The ListenAsyn() model could handily expose this as a new even type that is fired before ExitedCommandEvent. Too bad ExitedCommandEvent name is already taken as this is what it logically means.

One more compatible way could maybe be adding an optional "emit process exit immediately on exit" flag on ListenAsync. I wouldn't something clunkier though, as long as I could hook into that raw event directly when it comes.

Tyrrrz commented 1 year ago

Since the underlying Process fires Exited event, which is never seen by CliWrap because of forever blocking output streams, I think a good solution is exposing the Exited even in CliWrap.

How would that help if the execution would still block afterwards?

vivainio commented 1 year ago

How would that help if the execution would still block afterwards?

When I'm in "ListenAsync" mode, I could bail out from the await foreach loop when I see that signal. I could then cancel the rest with CancellationToken (I think)

Tyrrrz commented 1 year ago

Another thing you might try to do is to explicitly redirect ALL underlying streams inside your batch script. This way the handles shouldn't be inherited, I think.

start /min python 0> nul 1> nul 2> nul

This assumes you don't need the streams, of course. If you do (i.e., the user is expected to use the python window or something), then I think you'd be forced to use the raw Process class instead of CliWrap and remember to manually unsubscribe from standard output/error events after the process exits.

I found a related StackOverflow question with some extra context: https://stackoverflow.com/questions/36091748/how-do-i-use-the-start-command-without-inheriting-handles-in-the-child-process

fredatgithub commented 1 year ago

I had a similmar problem and in my mybatfile.cmd, I added an exit command in the last line and it solved my problem. If it can help.

Tyrrrz commented 1 year ago

After some deliberation on this, I decided to conclude that this behavior aligns with the design and all potential solutions on CliWrap's side are unfortunately too clunky to implement within the established model.

On the consumer's side, these are the main solutions, depending on the use case:

  1. If the child process isn't meant to interact with the console at all, then pipe all its streams to the null device. This will prevent it from inheriting parent handles and, as a result, CliWrap won't wait for the child process to terminate.

    start /min python 0> nul 1> nul 2> nul
  2. If the child process is meant to interact with the console, but not as part of the current script's lifetime, then the child process should be started in a detached state (i.e., in a separate console window). This will force the system to initialize separate streams for it and, as a result, CliWrap won't wait for the child process to terminate.

    start cmd /k start /min python
  3. If the child process is meant to interact with the console, and do it as part of the current script's lifetime, then the script should wait for the child process to exit:

    start /min /wait python