Add WithExitCondition feature #242

dmilosz · 2024-04-20T10:47:56Z

Closes #241

Example usage:

var cmd = Cli.Wrap("powershell")
    .WithArguments("Start-Process -NoNewWindow -FilePath \"powershell\" -ArgumentList \"'sleep 1000'\"; exit 0")
    .WithWaitingForOutputProcessing(false)
    .WithStandardOutputPipe(PipeTarget.ToDelegate(line => Console.WriteLine($"STDOUT {line}")))
    .WithStandardErrorPipe(PipeTarget.ToDelegate(line => Console.WriteLine($"STDERR {line}")));


var result = await cmd.ExecuteAsync();

There is no breaking change. Current implementations will get true by default and will go with old, standard flow.

CliWrap/Command.cs

Tyrrrz · 2024-04-20T14:48:08Z

I wonder if there's a way to implement this in another way -- by somehow instructing the child process not to share the stdout/err/in streams with its children. I wonder if it's possible? Is this issue specific to any OS?

…t implemented

dmilosz · 2024-04-21T09:13:10Z

I'm not aware of any method to tell grand children what streams to use. I think only the direct parent is capable of doing that.
The issue can also be reproduced with Ubuntu and bash, using nohup and background processing. Currently:

using CliWrap;

Console.WriteLine($"Program started at: {DateTime.Now.ToLongTimeString()}");

var cmd = Cli.Wrap("bash")
    .WithArguments(["-c", "nohup bash -c \"sleep 5; exit 0\" &"])
    .WithStandardOutputPipe(PipeTarget.ToDelegate(line => Console.WriteLine($"STDOUT {line}")))
    .WithStandardErrorPipe(PipeTarget.ToDelegate(line => Console.WriteLine($"STDERR {line}")));

var result = await cmd.ExecuteAsync();

Console.WriteLine($"Process started at: {result.StartTime.DateTime.ToLongTimeString()}");
Console.WriteLine($"Process finished with exit code: {result.ExitCode}");
Console.WriteLine($"Process finished at: {result.ExitTime.DateTime.ToLongTimeString()}");

Console.WriteLine($"Program finished at: {DateTime.Now.ToLongTimeString()}");

Output:

Program started at: 10:59:18
Process started at: 10:59:18
Process finished with exit code: 0
Process finished at: 10:59:18
Program finished at: 10:59:23

As you can see, the parent was finished almost instantly, but CliWrap waited for child process.

Now, after the change, using WithExitCondition(CommandExitCondition.ProcessExited):

using CliWrap;

Console.WriteLine($"Program started at: {DateTime.Now.ToLongTimeString()}");

var cmd = Cli.Wrap("bash")
    .WithArguments(["-c", "nohup bash -c \"sleep 5; exit 0\" &"])
    .WithExitCondition(CommandExitCondition.ProcessExited)
    .WithStandardOutputPipe(PipeTarget.ToDelegate(line => Console.WriteLine($"STDOUT {line}")))
    .WithStandardErrorPipe(PipeTarget.ToDelegate(line => Console.WriteLine($"STDERR {line}")));

var result = await cmd.ExecuteAsync();

Console.WriteLine($"Process started at: {result.StartTime.DateTime.ToLongTimeString()}");
Console.WriteLine($"Process finished with exit code: {result.ExitCode}");
Console.WriteLine($"Process finished at: {result.ExitTime.DateTime.ToLongTimeString()}");

Console.WriteLine($"Program finished at: {DateTime.Now.ToLongTimeString()}");

Output:

Program started at: 11:09:01
Process started at: 11:09:01
Process finished with exit code: 0
Process finished at: 11:09:01
Program finished at: 11:09:01

If you check processes after the program is finished, you can verify the bash child process is still running, after the program exits.

…it's the same as one passed by user

Tyrrrz

Can you please also add a test that fails with the current behavior, but works with the new option? You can probably use CliWrap.Tests.Dummy to spawn a child process.

CliWrap/CommandExitCondition.cs

CliWrap/Command.cs

dmilosz · 2024-04-22T11:36:07Z

Can you please also add a test that fails with the current behavior, but works with the new option? You can probably use CliWrap.Tests.Dummy to spawn a child process.

I've added 2 unit tests. They are not perfect, because the may fail unexpectedly when the machine is slow, since they depend on execution time. I don't have a better idea for a test which is 100% stable. However, child process is set to be running for 3 seconds so should be fine for most environments.

In the seconds test, if you remove .WithExitCondition(CommandExitCondition.ProcessExited); line, it will fail because child process will be definitely finished after the main process called by CLiWrap is done.

CliWrap.Tests/ExitConditionSpecs.cs

Tyrrrz · 2024-04-22T14:05:30Z

CliWrap.Tests/ExitConditionSpecs.cs

+
+        // Act
+        var executionStart = DateTime.UtcNow;
+        var result = await cmd.ExecuteAsync();


executionStart/executionFinish is already provided by result

Yes, but finish time is a different one. CliWrap sets ExitTime when the main process ends. When there are child processes attached to output, ExecuteAsync() is blocked until all children are completed. In this test, we want to measure time when the ExecuteAsync() was blocked for, so that we can verify whether it was waiting for child or not.

Exit time is set when the child process exits:

CliWrap/CliWrap/Utils/ProcessEx.cs

Lines 47 to 51 in 1cfa50e

_nativeProcess.Exited += (_, _) =>

{

ExitTime = DateTimeOffset.Now;

_exitTcs.TrySetResult(null);

};

It shouldn't be affected by the exit condition you've added, since that only affects whether CliWrap waits for pipes to clear or not.

Because your var executionFinish = ... statement appears right await cmd.ExecuteAsync(), it's essentially the same (slightly later) timestamp as the one provided by ExecutionResult.

CliWrap's ExitTime is always the same no matter what ExitCondition is set. So this test would basically always pass.

I'm confused in that case. Wouldn't ExitCondition.ProcessExit make the command finish earlier, assuming it spawns a grandchild process?

Tyrrrz · 2024-04-22T14:19:57Z

CliWrap.Tests/ExitConditionSpecs.cs

+        executionFinish
+            .Subtract(executionStart)
+            .Should()
+            .BeGreaterThanOrEqualTo(TimeSpan.FromSeconds(3));


Probably a more reliable way to test this would be to read the stdout. The sleep command prints Done. after it finishes, so you can use that to detect whether the grandchild process has finished or not.

Unfortunately it's not possible. The main process is finished almost instantly, and there is nothing we can do to catch the output after that. We can handle the output and wait for for child process to be finished, but then it breaks the whole point of doing this test, where we are interested in specific scenario when parent triggers child and then dies instantly.

I meant to test the current behavior (PipesClosed), wouldn't Done appear in the output since the command would only complete after the grandchild process exits?

Only if we make main process to wait for the child. In the CliWrap.Tests.Dummy I implemented run process command that doesn't wait for the child, because this is the behavior that can't be handled currrently.

In general it seems difficult to create a single test that works reliably on all environments including .NET framework and Linux. In Linux, we should somehow use nohup and background processing with bash to reproduce the issue, because the dummy app can't reproduce it.
On .NET Framework those tests also fail, but I don't know why yet.

I thought the issue is that, regardless of whether the child process waits for the grandchild or not, because the output streams are inherited and CliWrap waits for them to close, the command effectively finishes when all grandchildren exit.

dmilosz · 2024-04-29T09:29:04Z

I've implemented better solution in #243

dmilosz added 2 commits April 20, 2024 11:59

Added WaitForOutputProcessing feature

f0e7778

Don't use unnecessary WaitForEacit overload

6892a7e

Tyrrrz reviewed Apr 20, 2024

View reviewed changes

CliWrap/Command.cs Outdated Show resolved Hide resolved

Tyrrrz added the enhancement label Apr 20, 2024

Tyrrrz changed the title ~~Added WaitForOutputProcessing feature~~ Add WaitForOutputProcessing feature Apr 20, 2024

dmilosz added 2 commits April 21, 2024 10:55

Rename feature to CommandExitCondition

31529e1

Add fallback else statement for CommandExitCondition to throw when no…

3aca5e3

…t implemented

Fix cancellation token returned from the terminated command, so that …

a1dbfd8

…it's the same as one passed by user

Tyrrrz approved these changes Apr 21, 2024

View reviewed changes

Review fixes. Add unit tests for exit condition.

2505aa5

dmilosz changed the title ~~Add WaitForOutputProcessing feature~~ Add WithExitCondition feature Apr 22, 2024

Tyrrrz reviewed Apr 22, 2024

View reviewed changes

Unit tests fix.

1c8f829

dmilosz closed this Apr 29, 2024

Tyrrrz added invalid and removed enhancement labels Apr 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add WithExitCondition feature #242

Add WithExitCondition feature #242

dmilosz commented Apr 20, 2024

Tyrrrz commented Apr 20, 2024

dmilosz commented Apr 21, 2024

Tyrrrz left a comment

dmilosz commented Apr 22, 2024

Tyrrrz Apr 22, 2024

dmilosz Apr 22, 2024

Tyrrrz Apr 23, 2024

dmilosz Apr 24, 2024

Tyrrrz Apr 24, 2024

Tyrrrz Apr 22, 2024

dmilosz Apr 23, 2024

Tyrrrz Apr 23, 2024

dmilosz Apr 24, 2024

dmilosz Apr 24, 2024

Tyrrrz Apr 24, 2024

dmilosz commented Apr 29, 2024

	_nativeProcess.Exited += (_, _) =>
	{
	ExitTime = DateTimeOffset.Now;
	_exitTcs.TrySetResult(null);
	};

Add WithExitCondition feature #242

Add WithExitCondition feature #242

Conversation

dmilosz commented Apr 20, 2024

Tyrrrz commented Apr 20, 2024

dmilosz commented Apr 21, 2024

Tyrrrz left a comment

Choose a reason for hiding this comment

dmilosz commented Apr 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmilosz commented Apr 29, 2024