Check cancellation for task

[junjielu]

I think it is necessary to check if a task will be cancelled in a task cancellation scenario. The current code needs to actively check for cancellation at the top level, which I don't think is a reasonable approach.

Before:

let content = try await withTaskCancellation(id: CancelID.task, cancelInFlight: true) {
  let result = try await doSomeThing()
  try Task.checkCancellation()
  return result
}
await doSomeThingElse(content)

After:

let content = try await withTaskCancellation(id: CancelID.task, cancelInFlight: true) {
  try await doSomeThing()
}
await doSomeThingElse(content)

[mbrandonw]

Hi @junjielu, can you provide a bit more context about this change? What problem did you notice that this fixes? And can you write a test that fails without this change and passes with?

As far as I can tell they seem to be about equivalent as long as the work performed in try await operation() participates in cooperative cancellation.

[junjielu]

@mbrandonw Yes, I think I should have gone into more detail.
Regarding withTaskCancellation method, I think it makes sense that the method itself would actively throw an exception when the task is cancelled. The following scenario would be confusing for the user:

let taskA = try await withTaskCancellation(id: CancelID.A, cancelInFlight: true) {
  let result = try await doSomeThing() // Method won't check cancellation.
  print("Task A finished")
}

let taskB = try await withTaskCancellation(id: CancelID.B, cancelInFlight: true) {
  try await Task.sleep(nanoseconds: 5 * NSEC_PER_SEC) // `sleep` will check for cancellations internally.
  print("Task B finished")
}

let taskC = try await withTaskCancellation(id: CancelID.C, cancelInFlight: true) {
  try await doSomeThing() // Method won't check cancellation.
  print("doSomeThing finished")
  let result = try await doSomeThingElse() // Method will check cancellation.
  print("doSomeThingElse finished")
  return result
}
...

Task.cancel(id: CancelID.A) // "Task A finished" still printed
Task.cancel(id: CancelID.B) // "Task B finished" will not print
Task.cancel(id: CancelID.C) // "doSomeThing finished" will print, but "doSomeThingElse finished" won't

In this example you can see the significant difference, some async methods check for cancellation internally and some don't. So putting the matter of checking for cancellations inside the withTaskCancellation method reduces user misunderstandings.

After:

let taskA = try await withTaskCancellation(id: CancelID.A, cancelInFlight: true) {
  let result = try await doSomeThing() // Method won't check cancellation.
  print("Task A finished")
}

let taskB = try await withTaskCancellation(id: CancelID.B, cancelInFlight: true) {
  try await Task.sleep(nanoseconds: 5 * NSEC_PER_SEC) // `sleep` will check for cancellations internally.
  print("Task B finished")
}

let taskC = try await withTaskCancellation(id: CancelID.C, cancelInFlight: true) {
  try await doSomeThing() // Method won't check cancellation.
  print("doSomeThing finished")
  let result = try await doSomeThingElse() // Method will check cancellation.
  print("doSomeThingElse finished")
  return result
}
...

Task.cancel(id: CancelID.A) // "Task A finished" won't printed
Task.cancel(id: CancelID.B) // "Task B finished" won't print
Task.cancel(id: CancelID.C) // Nothing will print

[mbrandonw]

Hi @junjielu, I still do not think there is anything to fix here. As long as the async work you are invoking inside withTaskCancellation respects cooperative cancellation, everything should already work just fine. In your examples you seem to be theorizing calling functions that do not respect cooperative cancellation in order to show bad behavior.

However, even Swift's own async tools do not behave the way you want them to. They behave like our withTaskCancellation does.

For example, consider a non-cooperative sleep function:

func nonCooperativeSleep(duration: Duration) async throws {
  await Task { try? await Task.sleep(for: duration) }.value
}

This behaves like Task.sleep, except it does not cancel itself when the surrounding async context is cancelled.

If you then use this non-cooperative sleep in a Task and cancel that task like so:

let task = Task<Int, any Error> {
  try await nonCooperativeSleep(duration: .seconds(1))  // Does not cancel
  return 42
}
try? await Task.sleep(for: .seconds(0.5))  // Allow time to start the task
task.cancel()
let value = try await task.value
print(value)  // 42

…you will find that the value is returned from the task just fine and printed to the console.

If we were to take the change that you are proposing, then it would be the case that we need to litter try Task.checkCancellation() between essentially every line of every async context throughout our entire code base. And that is not how cooperative cancellation is meant to be dealt with in Swift. Instead, your async methods should take on the responsibility of checking for cancellation (such as the doSomeThing in your code above), rather than forcing that responsibility on library code that provides async tools.

So, I personally believe that things are correct as they are right now. If there is something I am not understanding though please do let me know.

[junjielu]

Hi @mbrandonw , I can see what you mean, I think where we're not agreeing right now is whether checkCancellation should happen inside the withTaskCancellation function or whether we should say let the external code use it manually.

I would think it depends on the expectations of the caller. For example with the function Task.sleep, the vast majority of coders would expect it to throw an error when the task is cancelled and not need to check for cancellation after await. For example, in Swift, when we use withTaskCancellationHandler function, the onCancel is executed immediately when the task is cancelled.

Then, when using withTaskCancellation I think the coder will also assume that it throws an error when the task is cancelled and does not need to check for cancellation multiple times when using it because not all async functions need to check for cancellation and that would make the code less readable.

try await withTaskCancellation(id: CancelID.Task, cancelInFlight: true) {
  let valueA = try await nonCooperativeSleep(duration: .seconds(1))
  try Task.checkCancellation()
  let valueB = try await nonCooperativeSleep(duration: .seconds(2))
  try Task.checkCancellation()
  let valueC = try await nonCooperativeSleep(duration: .seconds(3))
  try Task.checkCancellation()
  return valueA + valueB + valueC // The above three check cancellations I think would make boilerplate code
}

So it would be more in line with the coder's expectations if the cancellation error was thrown at the point of use, rather than checking for cancellation after every function that doesn't support task cancellation.

[OguzYuuksel]

Hi @junjielu, there is no reason to check if task is cancelled after operation is completed. Cancellation check should be inside the operation which is the actual side effect. During the implementation of side effect developer should consider checking cancellation before starting a heavy process. There is no value checking cancellation after heavy process since it's cancelled the caller is already forgot it. The main idea behind cancellation check is freeing resources if the operation is no longer needed.

[junjielu]

Hi @OguzYuuksel , I think what I'm trying to get is not when and where to check for cancellations. Let's forget all that, when we use a method called withTaskCancellation, shouldn't it throw a cancellation error when the task is cancelled? And actually this method doesn't throw an error with the current implementation, instead you have to explicitly call the cancellation check in the block, which I don't think is a sensible way of doing things.

[OguzYuuksel]

Hi @junjielu, I believe I got you but this is a side concept of withCheckedThrowingContinuation. Please see below tests, they may explain the concept better.

    func testFoo() async {
        let taskA = Task {
            try await withTaskCancellation(id: "taskA", cancelInFlight: true) {
                try? await Task.sleep(for: .seconds(1))
                return 1
             }
            print("taskA: completed")
        }

        let taskB = Task {
                try await withCheckedThrowingContinuation { continuation in
                    sleep(1)
                    continuation.resume(returning: 1)
                }
            print("taskB: completed")
        }

        let taskC = Task {
            // if you dont handle error the first operation will throw and task flow will stop which is expected
            do {
                try await withTaskCancellation(id: "taskC", cancelInFlight: true) {
                    try await Task.sleep(for: .seconds(2))
                    return 1
                }
            } catch {
                print("taskC - first task: throwed")
            }

            print("First task in the task flow is cancelled for taskC and this is handled in do catch taskC continues with the next operation")
        }

        let taskD = Task.detached {
            do {
                try await withCheckedThrowingContinuation { continuation in
                    sleep(2)
                    do {
                        try Task.checkCancellation()
                        continuation.resume(returning: 1)
                    } catch {
                        continuation.resume(throwing: error)
                    }
                }
            } catch {
                print("taskD - first task: throwed")
            }
            print("WARNING: Task flow is cancelled for taskD it is illegal to continue")
        }

        Task.cancel(id: "taskA")
        print("TaskA - task flow cancelled: ", taskA.isCancelled)
        taskB.cancel()
        print("TaskB - task flow cancelled: ", taskB.isCancelled)
        Task.cancel(id: "taskC")
        print("TaskC - task flow cancelled: ", taskC.isCancelled)
        taskD.cancel()
        print("TaskD - task flow cancelled: ", taskD.isCancelled)
        try? await Task.sleep(for: .seconds(5))
    }

from the above test all seems expected and matching with native Swift behavior.