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

Combining RedirectTo and Timeout #80

Closed
Elron1179 opened this issue Feb 2, 2021 · 1 comment
Closed

Combining RedirectTo and Timeout #80

Elron1179 opened this issue Feb 2, 2021 · 1 comment
Labels

Comments

@Elron1179
Copy link

Elron1179 commented Feb 2, 2021

I am having trouble using the Timeout option in combination with the RedirectTo-Method.
Example-code:

static async Task Main(string[] args)
  {
      var result = await Command.Run(
          executable: "cmd.exe", 
          arguments: new[] { "/c", "ping", "127.0.0.1", "/t" },
          options: options => options
              .Timeout(TimeSpan.FromSeconds(1)))
          .RedirectTo(new FileInfo(@"C:\Temp\shellTestOuput.txt"))
          .Task;
      Console.WriteLine($"result: {result.ExitCode}");

      Console.WriteLine("End. Press key...");
      Console.ReadKey();
  }

When I comment-out the RedirectTo line, the timeout is respected, but if I put it back in, the timeout is ignored.

Is this a bug or by design?

(If this a question better suited for stack-overflow, please say so and I'll move it there.)

Thank you.

Edit:
Environment:

  • .net 4.8
  • MedallionShell: 1.6.2
@madelson
Copy link
Owner

madelson commented Feb 4, 2021

@Elron1179 thanks for reporting. I looked into this and was able to reproduce.

The problem is that the command you are running launches cmd.exe which then launches PING.exe as a subprocess. When the Timeout goes off, it kills the cmd process, but this doesn't cause the PING process to exit. Because the PING process hasn't exited and because it inherited the console stream of cmd, StandardOutput stays open. Therefore the command won't exit because the redirection operation hasn't finished.

Luckily, there are (at least) two clean solutions that I can see. The first is to avoid the cmd indirection and call PING directly:

    await  Command.Run(
                executable: "PING.exe",
                arguments: new object[] { "127.0.0.1", "/t" },
                options: o => o.Timeout(TimeSpan.FromSeconds(1))
        )
        .RedirectTo(new FileInfo(@"C:\Temp\shellTestOuput.txt"))
        .Task;

The second is to do a graceful shutdown via signals in order to kill all processes on the current console.

    var command =  Command.Run(
                  executable: "cmd.exe",
                  arguments: new[] { "/c", "ping", "127.0.0.1", "/t" },
                  options: o => { } // no timeout
        )
        .RedirectTo(new FileInfo(@"C:\Temp\shellTestOuput.txt"));
     // manually set up a timeout with graceful shutdown
     Task.Delay(TimeSpan.FromSeconds(1))
        .ContinueWith(_ =>
        {
            if (!command.TrySendSignalAsync(CommandSignal.ControlC).Result
                || !command.Task.Wait(TimeSpan.FromSeconds(1))
            {
                // only resort to kill if sending the signal fails (it shouldn't) or if the command doesn't
                // promptly exit after the signal is sent
                command.Kill();
            }
        }

It would be nice if the library made it easier to figure this out/do this correctly without having to ask here. I've linked some enhancement ideas which I think could help in the future:

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

No branches or pull requests

2 participants