Better OperationCanceledExceptions

[taspeotis]

Details

Hi, thanks for your library. It has been useful for me. I have a very minor issue with how it throws OperationCanceledExceptions, however. It does not report which cancellationToken caused the operation to be canceled.

using System;
using System.Threading;
using CliWrap;

var timeSpan = TimeSpan.FromSeconds(1);
using var cancellationTokenSource = new CancellationTokenSource(timeSpan);
var cancellationToken = cancellationTokenSource.Token;

try
{
    // Replace with whatever command can take more than one second:
    await Cli.Wrap("sleep").WithArguments("5").ExecuteAsync(cancellationToken);
}
catch (OperationCanceledException operationCanceledException)
    when (operationCanceledException.CancellationToken == cancellationToken)
{
    Console.WriteLine("Canceled because of our cancellationToken!");
}

This should exit cleanly with a message saying Canceled because ... but it does not.

Unhandled exception. System.Threading.Tasks.TaskCanceledException: A task was canceled.
   at CliWrap.Utils.ProcessEx.WaitUntilExitAsync() in D:\a\CliWrap\CliWrap\CliWrap\Utils\ProcessEx.cs:line 125
   at CliWrap.Command.AttachAsync(ProcessEx process, CancellationToken cancellationToken) in D:\a\CliWrap\CliWrap\CliWrap\Command.cs:line 444
   at CliWrap.Command.AttachAsync(ProcessEx process, CancellationToken cancellationToken) in D:\a\CliWrap\CliWrap\CliWrap\Command.cs:line 471
   at Program.<Main>$(String[] args)
   at Program.<Main>(String[] args)
Command terminated by signal 6

I believe somewhere these constructors are not being used:

• https://docs.microsoft.com/en-us/dotnet/api/system.operationcanceledexception.-ctor?view=net-6.0#system-operationcanceledexception-ctor(system-threading-cancellationtoken)
• https://docs.microsoft.com/en-us/dotnet/api/system.operationcanceledexception.-ctor?view=net-6.0#system-operationcanceledexception-ctor(system-string-system-threading-cancellationtoken)
• https://docs.microsoft.com/en-us/dotnet/api/system.operationcanceledexception.-ctor?view=net-6.0#system-operationcanceledexception-ctor(system-string-system-exception-system-threading-cancellationtoken)

If this library were to use them, then I could tell which cancellationToken (mine or something else) caused the process to be canceled.

For the moment I can work around it by doing exactly one ExecuteAsync call per try/catch block and assuming any OperationCanceledException is from my cancellationToken.

[Tyrrrz]

If this library were to use them, then I could tell which cancellationToken (mine or something else) caused the process to be canceled.

Which other cancellationToken could cancel the process except for yours? Do you mean you have multiple linked tokens and want to differentiate which one was triggered?

[Tyrrrz]

Also, CliWrap does not construct OperationCanceledException manually, nor does it call ThrowIfCancellationRequested(), it only passes cancellationToken to underlying BCL methods.

With one exception, which is the push-based even stream model: https://github.com/Tyrrrz/CliWrap/blob/45a98c9eca3e261852755b916943f8d69c7cd2a1/CliWrap/EventStream/EventStreamCommandExtensions.cs#L133

But you don't seem to be using that in your example.

[Tyrrrz]

Actually, that's not entirely true, when the process is killed, then the waiting task is switched to canceled state here:

https://github.com/Tyrrrz/CliWrap/blob/45a98c9eca3e261852755b916943f8d69c7cd2a1/CliWrap/Utils/ProcessEx.cs#L61

Unfortunately, in that context, we don't have access to the cancellation token right now. Additionally, the process may have been killed by external factors as well, in theory.

[Tyrrrz]

To be honest, I still don't understand your exact use case from the example you've given me

[taspeotis]

Thanks, the use case is knowing when the operation was cancelled because of the cancellation token. I have various background services that run, and do things, and I want to know when they encounter an exception whether to retry indefinitely or whether it's because the background service is canceled.

e.g. with BackgroundService.ExecuteAsync

https://docs.microsoft.com/en-us/aspnet/core/fundamentals/host/hosted-services?view=aspnetcore-6.0&tabs=visual-studio#backgroundservice-base-class

ExecuteAsync(CancellationToken cancellationToken)
{
    while (true)
    {
        try
        {
            await DoSomeWorkAsync(cancellationToken);
        }
        catch (OperationCanceledException operationCanceledException)
            when (operationCanceledException.CancellationToken == cancellationToken)
        {
            return; // ASP.NET Core is shutting us down, this is expected; not an error.
        }
        catch (Exception exception)
        {
            _logger.Warning(exception, "Something went wrong");
            // Sleep for 3 sec or so and try again
        }
    }
}

And this works fine when DoSomeWorkAsync contains work involving HttpClient or EF Core which throw OperationCanceledException with the CancellationToken set to the cancellation token when they abort due to cancellation. In other places they throw TaskCanceledExceptions (which are OperationCanceledExceptions) without the CancellationToken set for things like timeouts.

https://docs.microsoft.com/en-us/dotnet/api/system.net.http.httpclient.sendasync?view=net-6.0#system-net-http-httpclient-sendasync(system-net-http-httprequestmessage-system-net-http-httpcompletionoption-system-threading-cancellationtoken)

Like I said it's a minor issue and I've seen you've taken some steps to preserve the cancellation token. I can work around it by wrapping CliWrap in a try/catch myself or being less trusting of the exceptions I'm given:

ExecuteAsync(CancellationToken cancellationToken)
{
    while (!cancellationToken.IsCancellationRequested)
    {
        try
        {
            await DoSomeWorkAsync(cancellationToken);
        }
        catch (OperationCanceledException operationCanceledException)
            when (cancellationToken.IsCancellationRequested)
        {
            return; // ASP.NET Core is shutting us down, this is expected; not an error.
        }
        catch (Exception exception)
        {
            _logger.Warning(exception, "Something went wrong");
            // Sleep for 3 sec or so and try again
        }
    }
}

Also, CliWrap does not construct OperationCanceledException manually, nor does it call ThrowIfCancellationRequested(), it only passes cancellationToken to underlying BCL methods.

I have had a quick look through the code and I suspect it might be because of linked cancellation token sources. The cancellation token from the linked source will be different from the one I call the method with.

Additionally, the process may have been killed by external factors as well, in theory.

That's not applicable here: if the process dies externally then you would not create an OperationCanceledException with a cancellation token specified. The cancellation token did not cancel the operation.

[Tyrrrz]

It should be fixed now. All OperationCanceledExceptions should have a valid token attached.