Rethrow CancellationError in TaskResult.init(catching:)

[stephencelis]

This came up on the Slack: it seems surprising that TaskResult can swallow cancellation errors. Instead it'd make sense for TaskResult to propagate them into the Effect.

This is technically a breaking change, where all TaskResult.inits must now be qualified with a try, and this can even look a lil confusing. At the very least the compiler can usually fix things automatically by inserting try.

Thoughts? Is there a better approach? Is there more surface area to think about? TaskResult.init(result:), for example?

[mbrandonw]

After discussing with @stephencelis we decided to re-target this to prerelease/1.0 since it is a breaking change and there doesn't seem to be a nice migration path (e.g. doing a @_disfavoredOverload that is deprecated didn't seem to work).

We'd love to hear people's opinions on this. I personally think it's a good idea, but also am a little put off seeing the try's show up everywhere in effects. If we had typed throws we could make it clear that only CancellationError's are thrown, but without that it seems weird to try on something whose whole purpose is to catch errors and send em back into the system.

[tgrapperon]

I don't know if you prefer to discuss here, in a discussion, or on Slack. I was initially reserved about the idea because:

• TaskResult is a kind of Result and the purpose of this type is to wrap failures. As a result(!), try [Task]Result(catching: {}) feels a little odd.
• CancellationError are "outer"/"external" errors, coming from the surrounding context's state. As a result, it seems possible to wrap or complement TaskResult in something that would catch the same error, making this behavior opt-in. For example the following naive function:

public func withCancellationError<Result>(
  perform operation: () async -> Result
) async throws -> Result {
  let result = await operation()
  try Task.checkCancellation()
  return result
}

I think this would cover the same cases as a throwing TaskResult, but maybe I'm missing some specific pattern

• CancellationError are typically swallowed in TCA. The fact that this type would throw only CancellationError and not other kind of errors would make it stand-out twice. It seems rather difficult to reason in large-scale projects, for example if you start to wrap this in a another rethrow function that will then effectively rethrow only CancellationErrors. At a distance, it may be very difficult to know that, and it may lead to useless/inappropriate error handling (even if it seems rather rare and exotic, I concur).
• The ceremony around a simple .task effect that produce a TaskResult is already boilerplaty: .task { await .action(TaskResult { try await f() } )}. Adding an additional try to this is not optimal (maybe an helper around effects producing TaskResults could help but this is another topic).

I'm playing a little the Devil's advocate though. It seems possible to catch the context's CancellationErrors by other means, so I'm not really seeing what kind of new pattern this change would allow¹, but maybe I'm missing something specific. I didn't see the discussion on Slack.

Footnotes

1. I can maybe imagine something where you would cancel an internal child Task in some work in TaskResult's closre, and yet want to let the error bubble up to the current context, but I can't imagine a concrete example. ↩

[rcarver]

This does seem correct but also really awkward.

1. The control-flow of throwing does seem like an improvement, though I doubt it's common to send multiple TaskResult actions in a row.
2. On the other hand, @tgrapperon makes an excellent point about Result not throwing.

I was advocating for this in Slack based on an experience where receiving a CancellationError in the reducer was surprising and noisy.

I was thinking this would fix it, but it no luck.

switch action {
case .response(.failure(CancellationError)): // ❌ Type 'CancellationError.Type' cannot conform to '_ErrorCodeProtocol'
  return .none
case .response(.failure(let error)):
  // handle error
}

But generally, I think anyone doing error handling in .failure(let error) would want to skip a CancellationError

So, just brainstorming another approach (and a very breaking change), what if TaskResult added case cancelled ?

Seeing the complexity that this raises I'm really not advocating one way or the other. There does seem to be something awkward about how cancellation is treated though. For another example, I sketched an idea for AsyncThrowing Stream in #1850

[tgrapperon]

@rcarver Wouldn't this kind of pattern matching work?

case .response(.failure(let error)) where error is CancellationError: …

[rcarver]

@tgrapperon aha yes thank you!

[stephencelis]

• TaskResult is a kind of Result and the purpose of this type is to wrap failures. As a result(!), try [Task]Result(catching: {}) feels a little odd.

Brandon and I agree that the outer try is odd for sure, and typed throws(CancellationError) would be a nice way to signal things, but the way I see it is that TaskResult is a result type specifically designed for routing async actions back to the store via Effect. Effect.{run,task,fireAndForget} have the behavior built-in that they drop cancellation errors, and it feels like sprinkling in a TaskResult or two to catch expected errors shouldn't alter the general flow of cancellation, nor should failure(CancellationError)s start showing up in your reducer, as they don't seem as actionable as other errors might be. This is just a feeling, though, and maybe someone can make me see why capturing and sending CancellationErrors through the reducer by default is a good thing that you'd want.

[tgrapperon]

About the outer try weirdness and the TaskResult ties to Effect, the following helper could maybe help making things a little leaner than .task, and tighten the API by baking the error's nature in it:

extension EffectTask {
  static func taskResult<Value: Sendable>(
    _ action: @escaping (TaskResult<Value>) -> Action,
    operation: @escaping @Sendable () async throws -> Value,
    onCancellation: (() async -> Action)? = nil
  ) -> EffectTask<Action> {
    if let onCancellation {
      return EffectTask<Action>.task {
        try await action(
          TaskResult(catching: operation)
        )
      } catch: { cancellationError in
        assert(cancellationError is CancellationError)
        return await onCancellation()
      }
    } else {
      return EffectTask<Action>.task {
        try await action(
          TaskResult(catching: operation)
        )
      }
    }
  }
}

You would use it as:

.taskResult(Action.entries) {
  try await dependency.fetchEntries()
}

// or 

.taskResult(Action.entries) {
  try await dependency.fetchEntries()
} onCancellation: {
  .fetchEntriesCancelled
}

in place of:

.task {
  try await .entries(
    TaskResult {
      try await dependency.fetchEntries()
    }
  )
}

// or

.task {
  try await .entries(
    TaskResult {
      try await dependency.fetchEntries()
    }
  )
} catch { _ in
  .fetchEntriesCancelled
}

It also seems that in this case, the throwing init of TaskResult could be made internal and only used by this .taskResult effect (and maybe some similar variants for .run and maybe .fireAndForget, albeit I don't really know under which form if the throwing init is internal)

[rcarver]

@stephencelis 👍 on all that. If I follow correctly, where in TCA would be responsible for ignoring a TaskResult.failure(CancellationError)?

maybe someone can make me see why capturing and sending CancellationErrors through the reducer by default is a good thing that you'd want.

Nope. I think if you care about CancellationError then catch and rethrow a specific error closer to where it occurs.

[lukeredpath]

Working through this in my codebase now and I think this change is important to make.

A common use case for cancellation is for when state is going away. TCA considers it to be an error to send actions back into a domain whose state is nil. Therefore when some state is going away, we might typically want to cancel some effects to prevent them from feeding back into the store.

With async tasks, throwing a cancellation error is what is expected but if you're using TaskResult this ends up with actions wrapping a TaskStore(CancellationError()) being sent into the store once the state has become nil.

I know you want to keep the surface area of the EffectTask API small but I am leaning heavily towards @tgrapperon's suggestion of a .taskResult effect that removes some of the boilerplate.

[lukeredpath]

I've decided to take this for a spin in our app - I've had to implement it as a static func helper because the compiler cannot disambiguate between the throwing and non-throwing initialisers, but I can easily find/replace these later. I am going to hold off on the .taskResult helper proposed above for now.

[davdroman]

This is probably naive but, couldn't we just add a cancelled case to TaskResult?

public enum TaskResult<Success: Sendable>: Sendable {
	/// A success, storing a `Success` value.
	case success(Success)

	/// A failure, storing an error.
	case failure(Error)

	/// A cancellation.
	case cancelled(CancellationError)
}

[lukeredpath]

Adding a cancelled case would not solve the problem of task results being fed back into the store when they shouldn't be.

[tgrapperon]

It also means that you will need to explicitly account for cancellation for each TaskResult:

case .onValues(.success(let values)):
  state.values = values
  return .none
case .onValues(.failure(let error)):
  return .fireAndForget {
    self.log.error(error)
  }
case .onValues(.cancelled):
  return .none

AFAIK, CancellationError is an empty struct, or at least opaque enough to not inspect it(This is indeed an empty struct), so the last case could be without associated value.
The throwing variant would make it opt-in by using a catch closure to .task & co.

[oronbz]

I totally vote for this PR, my console is spammed with cancellation errors, that are being printed/logged in the Reducer, and I definitely don't think another case for cancellation is the appropriate solution as it just adding more unneeded verbosity to the Reducer.

Regarding the additional try, I actually feels awkward to write await twice here, but only one try, so it totally makes sense syntax wise.

Other than that, whoever wants to do some logic for cancellation could just wrap the TaskResult initializer with do/catch and handle/rethrow there.

[stephencelis]

@lukeredpath Revisiting this now and with distance the change maybe seems the wrong direction. TaskResult can generally be thought of as glorified do { … } catch { … }, and so ignoring CancellationError seems a bit fraught, and could lead to confusing situations where state isn't cleaned up:

case .startRequest:
  state.isLoading = true
  return .run { await send(.response(TaskResult { … })) }

case .response:
  state.isLoading = false
  return .none

If TaskResult failed to capture cancellation errors, isLoading gets stuck to true.

Working through this in my codebase now and I think this change is important to make.

A common use case for cancellation is for when state is going away. TCA considers it to be an error to send actions back into a domain whose state is nil. Therefore when some state is going away, we might typically want to cancel some effects to prevent them from feeding back into the store.

With the new navigation tools, is this point now moot, since we automatically cancel these effects?

[stephencelis]

@lukeredpath I'm going to close this for now since it does seem problematic in hindsight, but definitely feel free to ping the PR again if we're missing something, and we can consider reopening and next steps.