TaskCanceledException.CancellationToken thrown by HttpClient does not equal to the original CancellationToken passed in

[yantang-msft]

Here is the simple code to reproduce the issue:

            var client = new HttpClient();
            var cts = new CancellationTokenSource(1000);
            var token = cts.Token;
            try
            {
                //await client.GetAsync("https://localhost:5001/weatherforecast", token);
                await client.SendAsync(new HttpRequestMessage(HttpMethod.Get, "https://localhost:5001/weatherforecast"), token);
            }
            catch (OperationCanceledException ex)
            {
                Console.WriteLine($"Same cancellation token: {token == ex.CancellationToken}");
            }

And the output Same cancellation token: False

Our code base has a lot of places to check if a OperationCanceledException is thrown because of passed in cancellationToken, e.g.,

public static bool IsOperationCanceledException(this Exception ex, CancellationToken cancellationToken) => ex is OperationCanceledException ocex && ocex.CancellationToken == cancellationToken;

catch (Exception ex) when (ex.IsOperationCanceledException(cancellationToken))

And this issue will cause exception handling issue for us.

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Here is the simple code to reproduce the issue:

            var client = new HttpClient();
            var cts = new CancellationTokenSource(1000);
            var token = cts.Token;
            try
            {
                //await client.GetAsync("https://localhost:5001/weatherforecast", token);
                await client.SendAsync(new HttpRequestMessage(HttpMethod.Get, "https://localhost:5001/weatherforecast"), token);
            }
            catch (OperationCanceledException ex)
            {
                Console.WriteLine($"Same cancellation token: {token == ex.CancellationToken}");
            }

And the output Same cancellation token: False

Our code base has a lot of places to check if a OperationCanceledException is thrown because of passed in cancellationToken, e.g.,

public static bool IsOperationCanceledException(this Exception ex, CancellationToken cancellationToken) => ex is OperationCanceledException ocex && ocex.CancellationToken == cancellationToken;

catch (Exception ex) when (ex.IsOperationCanceledException(cancellationToken))

And this issue will cause exception handling issue for us.

Author:	yantang-msft
Assignees:	-
Labels:	area-System.Net.Http, untriaged
Milestone:	-

[karelz]

@yantang-msft I believe this is by design. HttpClient.SendAsync links the token into another CTS:

runtime/src/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs

Lines 170 to 175 in 01b7e73

	private async Task<string> GetStringAsyncCore(HttpRequestMessage request, CancellationToken cancellationToken)
	{
	bool telemetryStarted = StartSend(request);
	bool responseContentTelemetryStarted = false;
	(CancellationTokenSource cts, bool disposeCts, CancellationTokenSource pendingRequestsCts) = PrepareCancellationTokenSource(cancellationToken);

runtime/src/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs

Lines 769 to 796 in 01b7e73

	private (CancellationTokenSource TokenSource, bool DisposeTokenSource, CancellationTokenSource PendingRequestsCts) PrepareCancellationTokenSource(CancellationToken cancellationToken)
	{
	// We need a CancellationTokenSource to use with the request. We always have the global
	// _pendingRequestsCts to use, plus we may have a token provided by the caller, and we may
	// have a timeout. If we have a timeout or a caller-provided token, we need to create a new
	// CTS (we can't, for example, timeout the pending requests CTS, as that could cancel other
	// unrelated operations). Otherwise, we can use the pending requests CTS directly.
	// Snapshot the current pending requests cancellation source. It can change concurrently due to cancellation being requested
	// and it being replaced, and we need a stable view of it: if cancellation occurs and the caller's token hasn't been canceled,
	// it's either due to this source or due to the timeout, and checking whether this source is the culprit is reliable whereas
	// it's more approximate checking elapsed time.
	CancellationTokenSource pendingRequestsCts = _pendingRequestsCts;
	bool hasTimeout = _timeout != s_infiniteTimeout;
	if (hasTimeout || cancellationToken.CanBeCanceled)
	{
	CancellationTokenSource cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, pendingRequestsCts.Token);
	if (hasTimeout)
	{
	cts.CancelAfter(_timeout);
	}
	return (cts, DisposeTokenSource: true, pendingRequestsCts);
	}
	return (pendingRequestsCts, DisposeTokenSource: false, pendingRequestsCts);
	}

Did you consider just checking token.IsCancellationRequested instead? That will tell you if your cancellation token triggered or not. Even in case both CT triggered by the time your code runs.

Looking at the TaskCancelledException._cancellationToken._source under debugger in your repro, it is indeed System.Threading.CancellationTokenSource.Linked2CancellationTokenSource.
The exception originates from this call stack:

System.Private.CoreLib.dll!System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(System.Threading.Tasks.Task task) Line 173	C#
System.Private.CoreLib.dll!System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(System.Threading.Tasks.Task task) Line 150	C#
System.Private.CoreLib.dll!System.Threading.Tasks.ValueTask<System.Net.Http.HttpConnection>.Result.get() Line 789	C#

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/TaskAwaiter.cs

Line 173 in 01b7e73

throw new TaskCanceledException(task);

It doesn't seem the code wants to care if the CT was wrapping other CTs and which one is the original source (it would have hard time to pick one when there are multiple in the tree).

[geoffkizer]

Is this how linked CTS's are intended to work? I would naively expect that using a linked CTS would still result in a TaskCanceledExecption that specifies which specific CTS was cancelled, not the linked one.

@stephentoub thoughts?

[yantang-msft]

@karelz Thanks for looking into it, the LinkedCancellationTokenSource is what I thought.
I think what you suggested by checking token.IsCancellationRequested is better than what we are doing now, though not perfect.
@geoffkizer the linked CTS appears to be the same as a normal CTS. Maybe it can expose some function to indicate the one initiated the cancellation, or maybe that's just too crazy.

[stephentoub]

I would naively expect that using a linked CTS would still result in a TaskCanceledExecption that specifies which specific CTS was cancelled, not the linked one.

That doesn't compose well. For example, consider code like:

void A(CancellationToken ct)
{
    using (var cts = CancellationTokenSource.CreateLinkedTokenSource(ct))
        B(cts.Token);
}

void B(CancellationToken ct)
{
    try
    {
        C(ct);
    }
    catch (OperationCanceledException oce) when (oce.CancellationToken == ct)
    {
        ...
    }
}

If method C does what you suggest and throws an exception with one of the underlying exceptions rather than the token passed in to C, B's behavior will break.

If you have a method like:

void M(CancellationToken cancellationToken)
{
    using (var cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken))
        Foo(cts.Token);
}

and its a goal to ensure that any cancellation exceptions that emerge from M due to cancellationToken having cancellation requested contain that passed in token, method M needs to do that exception management... it's the only code that has all the necessary context to know the intended behavior.

void M(CancellationToken cancellationToken)
{
    using (var cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken))
    {
        try
        {
            Foo(cts.Token);
        }
        catch (OperationCanceledException oce) when (oce.CancellationToken == cts.Token)
        {
            throw new OperationCanceledException(oce.Message, oce, cancellationToken);
        }
    }
}

HttpClient.HandleFailure can be updated accordingly.
#53133

[geoffkizer]

Makes sense.

It does seem like the implication here is: Every method that uses CreateLinkedTokenSource to add additional cancellation conditions on top of an existing CancellationToken probably needs to have the above logic to ensure that the proper CancellationToken is propagated in the exception.

Correct?

I have a feeling we have places that are not handling this correctly.

[stephentoub]

Correct?

If said method is exposed publicly from a library where a caller may have expectations around the token included in the exception, yes.

[geoffkizer]

Even if it's not exposed publicly, some internal logic may have expectations around the token in the exception. And even if it doesn't today, the code could evolve so that it does in the future.

It seems reasonable to say that a method that takes a CancellationToken should always report that CancellationToken in the TaskCanceledException if that CancellationToken is the cause of cancellation.

I suppose this could be avoided if we know it's never ever going to be needed, but that seems like the exception (lol) and not the rule.

[stephentoub]

that seems like the exception (lol) and not the rule.

In my experience, for internal code it's the minority case that the consumer cares about special-casing the specific token.