Should Swift warn about ineffective cancellation checks inside unstructured tasks?

Evolution Discussion
,
1

These checks can look valid while actually providing a false sense of cancellation safety:

Task {
  try Task.checkCancellation() // never throws here
  guard !Task.isCancelled else { return } // never returns here
}

A more realistic example is this:

setupBindingsTask?.cancel()
setupBindingsTask = Task {
  Task { [bindingService] in
    let greetingStream = await bindingService.greeting.asyncStream()
    for await greeting in greetingStream {
      guard !Task.isCancelled else { return } // <-- This cancelation check is not connected to setupBindingsTask
    }
  }
  Task { [bindingService] in
    let goodbyeStream = await bindingService.goodbye.asyncStream()
    for await goodbye in goodbyeStream {
      guard !Task.isCancelled else { return } // <-- This one as well
    }
  }
}

In this case, cancelling setupBindingsTask does not cancel the inner unstructured tasks.

This feels like an easy mistake to make, especially for developers that might think that child tasks are cancelled with their “parent task”.

2

Sort of unrelated, but even if it was "connected", this is likely not how you want to handle cancellation while looping over an AsyncSequence. It should be the sequence itself that terminates the loop upon cancellation, not a piece of code inside the loop (as that only runs when a new value is emitted).

Otherwise, the Task may take a long time to finish after being cancelled, or even be leaked forever if the async sequence never emits a new value!

Fortunately, AsyncStream already works like this (AFAIK all async sequences in the standard library do), and upon cancellation the loop ends and execution continues (in this case there's no more code in the task after the loop, though).

So the code works, but that cancellation check never runs[1], even when the task is "connected".

  1. Well, unless the task is cancelled in the extremely narrow window after a new value is emitted but before Task.isCancelled runs. ↩

3

Yes, true indeed.

Maybe it’s nitpicky but I’m still confused to why cancelation checks seems valid in an unstructured Task.

4

Maybe the stream example was a bad example, so here’s another:

cleanupTask?.cancel()
cleanupTask = Task {
  Task { [cache] in
    guard !Task.isCancelled else { return } // Not connected to cleanupTask
    await cache.removeExpiredItems()
    await cache.compactStorage()
  }

  Task { [imageStore] in
    guard !Task.isCancelled else { return } // Not connected to cleanupTask
    await imageStore.deleteTemporaryFiles()
    await imageStore.trimMemoryUsage()
  }

  Task { [sessionStore] in
    guard !Task.isCancelled else { return } // Not connected to cleanupTask
    await sessionStore.clearInactiveSessions()
    await sessionStore.persistPendingChanges()
  }
}

Without warnings this example seems like valid code unless you have knowledge about unstructured Tasks.

A fix suggestion would be the dream scenario here:

cleanupTask?.cancel()
cleanupTask = Task {
  await withDiscardingTaskGroup { group in
    group.addTask { [cache] in
      await cache.removeExpiredItems()
      await cache.compactStorage()
    }

    group.addTask { [imageStore] in
      await imageStore.deleteTemporaryFiles()
      await imageStore.trimMemoryUsage()
    }

    group.addTask { [sessionStore] in
      await sessionStore.clearInactiveSessions()
      await sessionStore.persistPendingChanges()
    }
  }
}
5

Basically I’m pointing out that this code should not be valid.

6

How would you characterize the situations in which such a hypothetical diagnostic would be produced? In the general case, since a Task can be cancelled "from within" via something like:

withUnsafeCurrentTask { $0?.cancel() }

simply applying a rule like "warn when an unstructured task is never stored and checks cancellation within its body" would presumably produce false positives (the existence of the "self cancellation" pattern could be completely opaque to the compiler). It's conceivable you could special case various things to reduce the risk of this, but then I suppose it's a question of whether a bespoke, potentially complex diagnostic pass is "worth it".

3 Likes
7

I see.

The only thing I can think of is an additional Task cancelation check for when you specifically would want to allow withUnsafeCurrentTask { $0?.cancel() }

With that in place we could check against Task.isCancelled in an unstructured Task and provide a warning:

warning: 'Task.isCancelled' inside a nested unstructured task does not observe cancellation of the enclosing task; use 'withTaskGroup' for structured cancellation, or 'Task.isCurrentCancelled' if checking the current task is intended

8

It would look something like this:

cleanupTask?.cancel()
cleanupTask = Task {
  Task { [cache] in
    guard !Task.isCurrentCancelled else { return } // Explicitly checks cancellation of the current task
    guard !Task.isCancelled else { return } // warning: 'Task.isCancelled' inside a nested unstructured task does not observe cancellation of the enclosing task; use 'withTaskGroup' for structured cancellation, or 'Task.isCurrentCancelled' if checking the current task is intended

    await cache.removeExpiredItems()
    await cache.compactStorage()
  }
}
9

Even then, it still only works at a single syntactic scope.

Between "even unmanaged tasks can be cancelled" and "outside the immediate lexical scope, all bets are off", I think there are simply too many caveats for this to be useful.

It would be better if instead the Task initializer did not allow discarding the task handle, which was/is being discussed here: Pitch: Remove discardableResult from throwing task initializers

10

Such diagnostic is pretty impossible to make in the general case, you don't know where and how a task handle was escaped.

I also am not sure of the value of this diagnostic, what does this actually give you? It's a very rare situation to not have to worry about cancellation to spend such huge amount of compiler effort to warn about. Rather, I'd say that yes you do need to consider if cancellation after all might have happened to a task.

11

Noted.

I also am not sure of the value of this diagnostic, what does this actually give you? It's a very rare situation to not have to worry about cancellation to spend such huge amount of compiler effort to warn about.

Well this topic is only a nitpic question really.
I just needed to know why as Swift Concurrency is to me the most elegant modern programming concept and it just feels like a rough edge that this mistake can be hidden in plain sight.

I guess it falls down to the relaxed rules of unstructured Tasks that’s all.

It would be better if instead the Task initializer did not allow discarding the task handle, which was/is being discussed here: Pitch: Remove discardableResult from throwing task initializers

Love that.

12

Just to make this clear: Child tasks are indeed cancelled with their parent tasks.

Task {} just does not create a child task, but an unstructured task. Child tasks are structured tasks.

3 Likes