Use `ThrowIfCancellationRequested` instead of simply returning

[Youssef1313]

@nohwnd What do you think?

cc @Evangelink @MarcoRossignoli

Fixes #5063

[Youssef1313]

Ping @nohwnd for review

[nohwnd]

What is the benefit that this change brings? Is it to make sure that we are respecting the cancellation and don't get stuck in subsequent code that handles the return?

Do you have some link to best practices that say why we should prefer one over the other? 🙂

[Youssef1313]

What is the benefit that this change brings? Is it to make sure that we are respecting the cancellation and don't get stuck in subsequent code that handles the return?

For our current code, it's mostly identical. But using ThrowIfCancellationRequested is the standard practice, and depending on the code present, there may be behavior difference between returning and throwing. Usually, callers of such cancellable methods will assume the method did complete successfully if it just returned normally. In situations that involve some caching (not something we do anywhere yet), it means that the caller may cache wrong info because it didn't differentiate between the method successfully completing its work and returning vs being cancelled.

[MarcoRossignoli]

my 2c

• if the code returns to the platform control I prefer a return for perf reason...anyway we're exiting and we cannot avoid any race conditions on that cancellation(throw or boolean)...but cancel means whatever will be the result it's partial and should be discarded
• if I'm a user of an extension I prefer return immediately for the same reason...if I didn't change any state nothing will change...if I change a local state I can throw or not...anyway before to use "that state" I should verify if it's valid or not

In general the platform handle the cancelled outcome/return value...so user can use or not the token correctly and bug aside we will be always correct(cooperative means that user could not cooperate).

[Youssef1313]

I personally don't see a perf concern here. However, for our specific case (at least for the implementation that exists today, there is not much difference between returning vs throwing. So putting on hold for now.

[Youssef1313]

At least for ConsumeAsync, while we are already just doing a return;, we will then get to here:

https://github.com/microsoft/testfx/blob/5bd7584ea1def491ac26f9f26fec5b9b78b38bb5/src/Platform/Microsoft.Testing.Platform/Messages/ChannelConsumerDataProcessor.cs#L57

then .NET will do this:

https://github.com/dotnet/runtime/blob/f6b93319242155a72801a7bb4bc92faac2ada1a3/src/libraries/System.Threading.Channels/src/System/Threading/Channels/SingleConsumerUnboundedChannel.cs#L72-L75

So, when awaiting the canceled task, we will get a TaskCanceledException (derives from OperationCanceledException) and we will get to the catch anyways.

For the rest of usages, I suspect that currently we may be doing more work due to not observing cancellation correctly.

[Youssef1313]

Closing stale PR, though I do think the changes proposed here make sense.