Piping task timeout

[dmilosz]

Closes #241

It resolves the issue, by allowing to specify max timeout to wait for piping to be finished. By doing this way, we can customize how long to wait after the process is finished. The code throws exception, when waiting for piping exceeded timeout, which can be cached by client and it can be properly handled.

Timeout also gives flexibility for the client to give enough time to handle the output, so that it's not lost for the cases when child process is started, but it's important to read all output of the main process.

It's also a lot easier to create nice unit test for the behavior. Unfortunately, it .NET Framework behaves differently in the same case, so I had to exclude it in unit test. In .NET Framework, when main process is exited but child process is still running, the Exited event is not fired for some reason.

Example from unit test:

[Fact(Timeout = 10000)]
public async Task I_can_execute_a_command_and_cancel_piping_when_command_is_finished_but_child_process_remains_running()
{
    // Arrange
    var cmd = Cli.Wrap(Dummy.Program.FilePath)
        .WithArguments(
            [
                "run",
                "process",
                "--path",
                Dummy.Program.FilePath,
                "--arguments",
                "sleep 00:00:15"
            ]
        )
        .WithStandardOutputPipe(PipeTarget.ToDelegate(_ => { }))
        .WithStandardErrorPipe(PipeTarget.ToDelegate(_ => { }))
        .WithPipingTimeout(TimeSpan.FromSeconds(1));

    // Act
    var executeAsync = async () => await cmd.ExecuteAsync();

    // Assert
    var ex = await Assert.ThrowsAnyAsync<PipesCancelledException>(
        async () => await executeAsync()
    );

    ex.ExitCode.Should().Be(0);
}

Without .WithPipingTimeout(TimeSpan.FromSeconds(1)) this test gets timeout after 10 seconds.

[Tyrrrz]

Hmm what if we make it possible to configure the command to not throw on cancellation and instead use ExecuteAsync(cancellationToken) where cancellationToken is your timeout.

[dmilosz]

When executing a command, it's impossible to know how long it will take to complete. If a cancellation token is set and fired while the command is still running, it will force pipingTask an immediate finish, potentially resulting in lost output logs.

It's important to note that the cancellation token will only work if it's fired manually by the client, such as in reaction to some expected output on process finish. Otherwise, using CancelAfter could result in lost output.

To avoid this, it's better to allow some time for the pipingTask to finish its work, especially when there is traffic in the output. If the client knows that the command has no running child processes, the pipingTimeout should be left empty. However, if the client knows that there is a child process started that will block the output and the command may produce a large output, the client can decide how long to wait before timing out.

[Tyrrrz]

So is the difference from the current cancellation behavior such that the pipingTimeout doesn't kill the process, while normal cancellation does?

[dmilosz]

When you use graceful/forceful cancellation tokens, they send interrupt/kill signal to process, then forceful cancellation token is used to finish piping task. So you could use forceful cancellation token to cancel the process and piping, but then

• you can't get exit code, start time and exit time of the process
• you don't know whether the main process was finished or not
• there is no way to notify when the main process is finished, but stdout is locked

This PR resolves above issues.

[dmilosz]

Hi @Tyrrrz, what do you think about merging this request? We use it in our production for some time and it works fine. If you'd like to, I can provide more tests, but the behavior added here doesn't break existing features.

[Tyrrrz]

Hi @dmilosz

Sorry, I can't find the time to dedicate to this yet. I have it on my todo list and I will come back to it when I'm a bit less busy.

[Tyrrrz]

Hi @dmilosz

Sorry for the wait.

I think instead of a timeout, it's actually better to have an on/off setting, like you originally suggested in the linked issue. I doubt there's value in timeouts other than 0 and infinite (null).

Also, for consistency with WithValidation(...), I think it should be an enum:

// Wait for pipes, default
Cli.Wrap(...).WithCompletion(CommandCompletion.PipesClosed);

// Don't wait for pipes, only for process exit
Cli.Wrap(...).WithCompletion(CommandCompletion.ProcessExited);

[dmilosz]

Parent process may write a lot of logs to std output. If we just wait for process exit, we can possibly not handle all logs from it. With timeout, I can set it to X seconds, so I make sure that when child processes are launched, ale logs from parent process are handled (if my X seconds are enough to catch it).

[Tyrrrz]

I see what you mean. I don't like introducing a timeout because, like you said, it does not provide any guarantees that the streams will be fully read by then. Set it too low and you may still miss some data, set it too high and you might wait too long.

It might also be extra problematic if the child process does not write much or anything to the output streams, because then they may never be flushed until the child process exits too. This depends on the platform, though.

Overall, I believe there are too many problematic aspects about this implementation that makes it difficult to support it as a first class feature in the library. Maybe we can find a better solution eventually.

[dmilosz]

This timeout is only relevant in cases where the main process quickly finishes but launches other processes that continue running indefinitely. It's not a breaking change and won't affect any existing implementations. Instead, it adds support for this specific scenario, allowing users to wait for output if they expect the main process to generate a significant amount of logs.

I don't think it's problematic, but I understand the use case is rare one.