sync: fix CancellationToken failing to cancel the ready futures

[stepantubanov]

When token is already cancelled, provided future should not be polled.

Motivation

Example: We have a loop where we get something from a stream but want to exit in case of shutdown:

loop {
  let Some(item) = shutdown.run_until_cancelled(stream.next()).await else {
     info!("Exit due to shutdown");
     break;
  };
  ...
}

With current logic, as long as the stream is yielding new items, the loop is not terminating even after the token is cancelled, which seems like a bug to me.

Solution

I've changed the order of if branches to make it check the token before polling the future.

[ADD-SP]

Overall looks good to me.

[ADD-SP]

Thanks!

[lucab]

Casual observer here, but I noticed that here is an explicit comment here about the bias of this polling-order:

tokio/tokio-util/src/sync/cancellation_token.rs

Lines 274 to 283 in 0a3fe46

	pub async fn run_until_cancelled<F>(&self, fut: F) -> Option<F::Output>
	where
	F: Future,
	{
	pin_project! {
	/// A Future that is resolved once the corresponding [`CancellationToken`]
	/// is cancelled or a given Future gets resolved. It is biased towards the
	/// Future completion.
	#[must_use = "futures do nothing unless polled"]
	struct RunUntilCancelledFuture<'a, F: Future> {

And I think this bias was explicitly requested here: #6618 (comment)

As an additional random thought, if the problem is related to calling run_until_cancelled() in a loop on a token that is already cancelled, does it maybe make sense to keep the current order bias but check for is_cancelled() upfront before (conditionally) constructing the RunUntilCancelledFuture?

[ADD-SP]

And I think this bias was explicitly requested here: #6618 (comment)

This is a good finding, I didn't noticed this, and I think we should also change this comment because:

1. The docs doesn't mention this biased behavior.
2. This biased behavior is likely surprising the downstream.

I'm also open for keeping the existing biased behavior for better performance of happy path, but writing a warning about this behavior and suggesting a recommended approach to deal with this.

[lucab]

As an additional random thought, if the problem is related to calling run_until_cancelled() in a loop on a token that is already cancelled, does it maybe make sense to keep the current order bias but check for is_cancelled() upfront before (conditionally) constructing the RunUntilCancelledFuture?

I tried to implement this and check it against the newly added test. I posted my changes to https://github.com/stepantubanov/tokio/pull/1/files in case you want to go that way.

[stepantubanov]

@lucab Thanks, seems like a good way to preserve the bias while correcting the behavior for this scenario.

there is a brand new fix

[ADD-SP]

@stepantubanov @lucab Thank you both for your contributions, this is a better solution that I didn't realize before.