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

Add WithExitCondition feature #242

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions CliWrap.Tests.Dummy/Commands/RunProcessCommand.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
using System;
using System.Diagnostics;
using System.Threading.Tasks;
using CliFx;
using CliFx.Attributes;
using CliFx.Infrastructure;

namespace CliWrap.Tests.Dummy.Commands;

[Command("run process")]
public class RunProcessCommand : ICommand
{
[CommandOption("path")]
public string FilePath { get; init; } = string.Empty;

[CommandOption("arguments")]
public string Arguments { get; init; } = string.Empty;

public ValueTask ExecuteAsync(IConsole console)
{
var startInfo = new ProcessStartInfo
{
FileName = FilePath,
Arguments = Arguments,
RedirectStandardInput = true,
RedirectStandardOutput = true,
RedirectStandardError = true,
UseShellExecute = false,
CreateNoWindow = true
};

var process = new Process();
process.StartInfo = startInfo;
process.Start();

console.Output.WriteLine(process.Id);

return default;
}
}
80 changes: 80 additions & 0 deletions CliWrap.Tests/ExitConditionSpecs.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
using System;
using System.Diagnostics;
using System.Text;
using System.Threading.Tasks;
using FluentAssertions;
using Xunit;

namespace CliWrap.Tests;

public class ExitConditionSpecs()
{
[Fact(Timeout = 15000)]
public async Task I_can_execute_a_command_that_creates_child_process_reusing_standard_output_and_finish_after_child_process_exits()
{
// Arrange
var cmd = Cli.Wrap(Dummy.Program.FilePath)
.WithArguments(
[
"run",
"process",
"--path",
Dummy.Program.FilePath,
"--arguments",
"sleep 00:00:03"
]
)
.WithStandardOutputPipe(PipeTarget.ToDelegate(_ => { }))
.WithStandardErrorPipe(
PipeTarget.ToDelegate(line => Console.WriteLine($"Error: {line}"))
);

// Act
var executionStart = DateTime.UtcNow;
var result = await cmd.ExecuteAsync();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

executionStart/executionFinish is already provided by result

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exit time is set when the child process exits:

_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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var executionFinish = DateTime.UtcNow;

// Assert
executionFinish
.Subtract(executionStart)
.Should()
.BeGreaterThanOrEqualTo(TimeSpan.FromSeconds(3));
Comment on lines +38 to +41
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

[Fact(Timeout = 15000)]
public async Task I_can_execute_a_command_that_creates_child_process_resuing_standard_output_and_finish_instantly_after_main_process_exits()
{
// Arrange
int childProcessId = -1;
var cmd = Cli.Wrap(Dummy.Program.FilePath)
.WithArguments(
[
"run",
"process",
"--path",
Dummy.Program.FilePath,
"--arguments",
"sleep 00:00:03"
]
)
.WithStandardOutputPipe(
PipeTarget.ToDelegate(line => int.TryParse(line, out childProcessId))
)
.WithStandardErrorPipe(
PipeTarget.ToDelegate(line => Console.WriteLine($"Error: {line}"))
)
.WithExitCondition(CommandExitCondition.ProcessExited);

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

var process = Process.GetProcessById(childProcessId);

// Assert
executionFinish.Subtract(executionStart).Should().BeLessThan(TimeSpan.FromSeconds(3));

process.HasExited.Should().BeFalse();
}
}
40 changes: 36 additions & 4 deletions CliWrap/Command.Execution.cs
Original file line number Diff line number Diff line change
Expand Up @@ -219,11 +219,17 @@ private async Task<CommandResult> ExecuteAsync(
.Register(process.Interrupt)
.ToAsyncDisposable();

// Create token to cancel piping when process is finished and we don't need to finish piping
using var outputProcessingCts = new CancellationTokenSource();
using var pipeStdOutErrCts = CancellationTokenSource.CreateLinkedTokenSource(
forcefulCancellationToken,
outputProcessingCts.Token
);
// Start piping streams in the background
var pipingTask = Task.WhenAll(
PipeStandardInputAsync(process, stdInCts.Token),
PipeStandardOutputAsync(process, forcefulCancellationToken),
PipeStandardErrorAsync(process, forcefulCancellationToken)
PipeStandardOutputAsync(process, pipeStdOutErrCts.Token),
PipeStandardErrorAsync(process, pipeStdOutErrCts.Token)
);

try
Expand All @@ -239,8 +245,34 @@ private async Task<CommandResult> ExecuteAsync(
// If the pipe is still trying to transfer data, this will cause it to abort.
await stdInCts.CancelAsync();

// Wait until piping is done and propagate exceptions
await pipingTask.ConfigureAwait(false);
if (CommandExitCondition == CommandExitCondition.PipesClosed)
{
try
{
// Wait until piping is done and propagate exceptions
await pipingTask.ConfigureAwait(false);
}
catch (OperationCanceledException)
{
// throw original token if it was the source of cancel
forcefulCancellationToken.ThrowIfCancellationRequested();
throw;
}
}
else if (CommandExitCondition == CommandExitCondition.ProcessExited)
{
try
{
// Cancel piping if we don't need to wait for it
await outputProcessingCts.CancelAsync();
await pipingTask.ConfigureAwait(false);
}
catch (OperationCanceledException) { }
}
else
{
throw new NotImplementedException($"{CommandExitCondition} is not implemented.");
}
}
// Swallow exceptions caused by internal and user-provided cancellations,
// because we have a separate mechanism for handling them below.
Expand Down
54 changes: 43 additions & 11 deletions CliWrap/Command.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ public partial class Command(
CommandResultValidation validation,
PipeSource standardInputPipe,
PipeTarget standardOutputPipe,
PipeTarget standardErrorPipe
PipeTarget standardErrorPipe,
CommandExitCondition commandExitCondition
) : ICommandConfiguration
{
/// <summary>
Expand All @@ -35,7 +36,8 @@ public Command(string targetFilePath)
CommandResultValidation.ZeroExitCode,
PipeSource.Null,
PipeTarget.Null,
PipeTarget.Null
PipeTarget.Null,
CommandExitCondition.PipesClosed
) { }

/// <inheritdoc />
Expand All @@ -50,6 +52,9 @@ public Command(string targetFilePath)
/// <inheritdoc />
public Credentials Credentials { get; } = credentials;

/// <inheritdoc />
public CommandExitCondition CommandExitCondition { get; } = commandExitCondition;
dmilosz marked this conversation as resolved.
Show resolved Hide resolved

/// <inheritdoc />
public IReadOnlyDictionary<string, string?> EnvironmentVariables { get; } =
environmentVariables;
Expand Down Expand Up @@ -80,7 +85,8 @@ public Command WithTargetFile(string targetFilePath) =>
Validation,
StandardInputPipe,
StandardOutputPipe,
StandardErrorPipe
StandardErrorPipe,
CommandExitCondition
);

/// <summary>
Expand All @@ -101,7 +107,8 @@ public Command WithArguments(string arguments) =>
Validation,
StandardInputPipe,
StandardOutputPipe,
StandardErrorPipe
StandardErrorPipe,
CommandExitCondition
);

/// <summary>
Expand Down Expand Up @@ -147,7 +154,8 @@ public Command WithWorkingDirectory(string workingDirPath) =>
Validation,
StandardInputPipe,
StandardOutputPipe,
StandardErrorPipe
StandardErrorPipe,
CommandExitCondition
);

/// <summary>
Expand All @@ -164,7 +172,8 @@ public Command WithCredentials(Credentials credentials) =>
Validation,
StandardInputPipe,
StandardOutputPipe,
StandardErrorPipe
StandardErrorPipe,
CommandExitCondition
);

/// <summary>
Expand Down Expand Up @@ -196,7 +205,8 @@ public Command WithEnvironmentVariables(
Validation,
StandardInputPipe,
StandardOutputPipe,
StandardErrorPipe
StandardErrorPipe,
CommandExitCondition
);

/// <summary>
Expand Down Expand Up @@ -226,7 +236,26 @@ public Command WithValidation(CommandResultValidation validation) =>
validation,
StandardInputPipe,
StandardOutputPipe,
StandardErrorPipe
StandardErrorPipe,
CommandExitCondition
);

/// <summary>
/// Creates a copy of this command, setting the exit condition to the specified value.
/// </summary>
[Pure]
public Command WithExitCondition(CommandExitCondition commandExitCondition) =>
new(
TargetFilePath,
Arguments,
WorkingDirPath,
Credentials,
EnvironmentVariables,
Validation,
StandardInputPipe,
StandardOutputPipe,
StandardErrorPipe,
commandExitCondition
);

/// <summary>
Expand All @@ -243,7 +272,8 @@ public Command WithStandardInputPipe(PipeSource source) =>
Validation,
source,
StandardOutputPipe,
StandardErrorPipe
StandardErrorPipe,
CommandExitCondition
);

/// <summary>
Expand All @@ -260,7 +290,8 @@ public Command WithStandardOutputPipe(PipeTarget target) =>
Validation,
StandardInputPipe,
target,
StandardErrorPipe
StandardErrorPipe,
CommandExitCondition
);

/// <summary>
Expand All @@ -277,7 +308,8 @@ public Command WithStandardErrorPipe(PipeTarget target) =>
Validation,
StandardInputPipe,
StandardOutputPipe,
target
target,
CommandExitCondition
);

/// <inheritdoc />
Expand Down
18 changes: 18 additions & 0 deletions CliWrap/CommandExitCondition.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
namespace CliWrap;

/// <summary>
/// Strategy used for identifying the end of command exectuion.
/// </summary>
public enum CommandExitCondition
{
/// <summary>
/// Command execution is considered finished when the process exits and all standard input and output streams are closed.
/// </summary>
PipesClosed = 0,

/// <summary>
/// Command execution is considered finished when the process exits, even if the process's standard input and output streams are still open,
/// for example after being inherited by a grandchild process.
/// </summary>
ProcessExited = 1
}
5 changes: 5 additions & 0 deletions CliWrap/ICommandConfiguration.cs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,11 @@ public interface ICommandConfiguration
/// </summary>
Credentials Credentials { get; }

/// <summary>
/// Strategy used for veryfing the end of command exectuion.
/// </summary>
public CommandExitCondition CommandExitCondition { get; }

/// <summary>
/// Environment variables set for the underlying process.
/// </summary>
Expand Down