TaskCanceledException has wrong CancellationToken

[mknejp]

Expected Behavior

When calling any of the asynchronous methods on RestClient (like ExecuteTaskAsync) with a user-provided CancellationToken the resulting TaskCancelledException's CancellationToken property should be the same as the provided token.

Actual Behavior

The exception's CancellationToken property is not equal to the provided token. In fact, the token stored in the exception even reports IsCancellationRequested as false.

This can cause problems with task cancellation where a TaskCanceledException with the wrong token may transition a task to the faulted state instead of the canceled state and potentially crash the application with an unhandled exception.

Steps to Reproduce the Problem

1. Call ExecuteTaskAsync on a RestClient instance and provide a custom CancellationToken parameter.
2. Cancel the operation before it finishes.
3. Compare the TaskCancelledException.CancellationToken property to the provided token and see that they do not match.

Specifications

• Version: 106.6.9
• Platform: .NET Framework
• Subsystem:

[jnyrup]

If your expected behavior can be expressed as this test

public async Task TaskCanceledException_Should_Contain_Provided_CancellationToken()
{
    Uri baseUrl = new Uri("http://localhost:8888/");

    RestClient client = new RestClient(baseUrl);
    RestRequest request = new RestRequest("");

    var token = new CancellationToken(true);
    try
    {
        await client.ExecuteTaskAsync(request, token);
    }
    catch (TaskCanceledException tce)
    {
        Assert.AreEqual(token, tce.CancellationToken);
    }
}

Then the problem is that TaskCompletionSource.TrySetCanceled() is used instead of TaskCompletionSource.TrySetCanceled(CancellationToken cancellationToken)

I'm not familiar enough with CancellationToken to see if that change is desired, but that is what's done in the AsyncEx library.

The downside is, that the overload taking a CancellationToken was internal until net46 and RestSharp targets net452.

https://github.com/theraot/Theraot/issues/103 Uses a cached reflection delegate to enable it for net45 targets.

[mknejp]

That test does indeed show the expected behavior. That is quite the effort to get it working pre net46. Very unfortunate.

As I mentioned before, not including the correct token causes a task to transition to the faulted state instead of the cancelled state. It is explicitly mentioned here https://docs.microsoft.com/en-us/dotnet/standard/parallel-programming/task-cancellation

If the token's IsCancellationRequested property returns false or if the exception's token does not match the Task's token, the OperationCanceledException is treated like a normal exception, causing the Task to transition to the Faulted state

RestSharp ticks both boxes: the IsCancellationRequested property does return false and it is the wrong token. So in the worst case this can crash an application with an unhandled exception when the OperationCanceledException bubbles up and there is nobody waiting on the task.

[alexeyzimarev]

So, what can we do about it? Bump to 4.6.1?

[jnyrup]

As I see it:

• Leave it as it is - does not solve the bug.
• Bump target framework to net46 + makes the necessary overload TrySetCanceled available to solve the bug for both net46 and netstandard2.0 - Prevents people still using net452 from upgrading.
• Add net46 as an extra target framework + and use the overloaded TrySetCanceled for net46 and netstandard2.0 - does not fix the problem for users of net452 target
• Add the quick from Theraot to net452 target framework + use the overloaded TrySetCanceled for both net452 and netstandard2.0 \? Need to make sure that it works as expected when a net46 project consumes the net452 package
• Combine the two previous: add net46 target and Theraot quirk to net452 target framework + Avoids reflection for net46

[alexeyzimarev]

Yeah, adding the new target could be an option, plus conditional pragmas.

[alexeyzimarev]

In fact, I plan to refactor all async code in RestSharp to use GetResponseAsync and GetRequestStreamAsync. A lot of the existing code will be therefore removed. I am not sure if this change will fix this issue, but it might. So, I'd like to finish what I've planned first and then we get back to it.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[repo-ranger[bot]]

⚠️ This issue has been marked wontfix and will be closed in 3 days

[alexeyzimarev]

Found out that those async methods of WebRequest are fake, really. I haven't decided if I need to prioritize this or pursue the HttpClient migration.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[repo-ranger[bot]]

⚠️ This issue has been marked wontfix and will be closed in 3 days