Skip to content
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

PipeTarget.ToDelegate blocks forever in StreamReader #185

Closed
vivainio opened this issue Dec 31, 2022 · 11 comments
Closed

PipeTarget.ToDelegate blocks forever in StreamReader #185

vivainio opened this issue Dec 31, 2022 · 11 comments

Comments

@vivainio
Copy link

Version

ver 3.5.0

Details

This is a spin off issue from: #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

  • use cmd.ListenAsync() like here:
  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 vivainio added the bug label Dec 31, 2022
@vivainio
Copy link
Author

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
Copy link
Owner

Tyrrrz commented Dec 31, 2022

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

@vivainio
Copy link
Author

vivainio commented Dec 31, 2022

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
Copy link
Owner

Tyrrrz commented Jan 1, 2023

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
Copy link
Owner

Tyrrrz commented Jan 6, 2023

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

  • test.cmd:
start /min python
  • Test method:
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
Copy link
Author

vivainio commented Jan 7, 2023

@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
Copy link
Owner

Tyrrrz commented Jan 8, 2023

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
Copy link
Author

vivainio commented Jan 8, 2023

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
Copy link
Owner

Tyrrrz commented Jan 17, 2023

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
Copy link

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 Tyrrrz added the wontfix label Aug 7, 2023
@Tyrrrz
Copy link
Owner

Tyrrrz commented Aug 7, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants