Require catch block on EffectPublisher.tasks that throw

[patch-benjamin]

I've noticed on a few PRs at work recently that there is a potential for hanging UI bugs to occur with the optional provision of a catch block on EffectPublisher.task.

I propose two small (breaking) changes to EffectPublisher:

1. Update EffectPublisher.task to not provide a default nil value for the catch block.
2. Create a non-throwing EffectPublisher.task function that doesn't require a catch block.

The result of these changes would be to make it clearer to developers that errors are thrown and allow them to explicitly specify how they want to handle those thrown errors -- even if that is catch: nil.
This may be a moot suggestion with the direction of TCA moving away from Combine, but figured I'd throw it out there for discussion at the least since I assume similar functionality will be available in the next major version.

If you don't see how a thrown error could leave the UI in a hanging state, here's an example from our code base:

1. User taps a button that requires a network call
2. Reducer update State to be Loading
3. Reducer makes network call but doesn't provide a catch block
4. Network call throws for some reason (say a custom network stack that throws errors you're unaware of)
   -> Result: The user gets stuck in a hanging Loading state rather than an error state

I know the dev has ultimate responsibility for handling the catch block when he calls try in his code, but I like the idea of making it more in your face that an error could be thrown and that you are ignoring the catching of that error.

The other benefit of this change is that users can extend EffectPublisher and implement their own version of func task to give the catch block a default value if they want. Whereas the current approach does not allow users to remove that default value in an effort to force developers to explicitly define the catch block.

Here's what the code change to the API could look like:

extension EffectPublisher where Failure == Never {

    // The current form of `task` would be removed

//    public static func task(
//      priority: TaskPriority? = nil,
//      operation: @escaping @Sendable () async throws -> Action,
//      catch handler: (@Sendable (Error) async -> Action)? = nil,
//      file: StaticString = #file,
//      fileID: StaticString = #fileID,
//      line: UInt = #line
//    ) -> Self {
//        ...
//    }

    // Split `task` out into two different functions. One that throws and requires a catch block to be provided, and another that doesn't throw.

    // Throwing `task`
    public static func task(
      priority: TaskPriority? = nil,
      operation: @escaping @Sendable () async throws -> Action,
      catch handler: (@Sendable (Error) async -> Action)?,
      file: StaticString = #file,
      fileID: StaticString = #fileID,
      line: UInt = #line
    ) -> Self {
        ...
    }

    // Non-throwing `task`
    public static func task(
      priority: TaskPriority? = nil,
      operation: @escaping @Sendable () async -> Action,
      file: StaticString = #file,
      fileID: StaticString = #fileID,
      line: UInt = #line
    ) -> Self {
        ...
    }


    // The suggestion would also apply to `run`

//    public static func run(
//      priority: TaskPriority? = nil,
//      operation: @escaping @Sendable (Send<Action>) async throws -> Void,
//      catch handler: (@Sendable (Error, Send<Action>) async -> Void)? = nil,
//      file: StaticString = #file,
//      fileID: StaticString = #fileID,
//      line: UInt = #line
//    ) -> Self {
//
//    }

    public static func run(
      priority: TaskPriority? = nil,
      operation: @escaping @Sendable (Send<Action>) async throws -> Void,
      catch handler: (@Sendable (Error, Send<Action>) async -> Void)?, // remove the `= nil` for a throwing `run`
      file: StaticString = #file,
      fileID: StaticString = #fileID,
      line: UInt = #line
    ) -> Self {
        ...
    }
    public static func run(
      priority: TaskPriority? = nil,
      operation: @escaping @Sendable (Send<Action>) async -> Void, // non-throwing `run` version without the `catch` requirement
      file: StaticString = #file,
      fileID: StaticString = #fileID,
      line: UInt = #line
    ) -> Self {
        ...
    }
}

And then using a throwing task would change from:

return .task {
    try someThrowingFunction()
    return .successfulAction
}

to

return .task(operation: {
    try someThrowingFunction()
    return .successfulAction
}, catch: nil)

Thus forcing developers to specify that they don't want to do anything for the catch block.

I also wouldn't be opposed to taking it one step further and making the catch block non-optional but returning an optional Action? like so:

    // Throwing `task`
    public static func task(
      priority: TaskPriority? = nil,
      operation: @escaping @Sendable () async throws -> Action,
      catch handler: (@Sendable (Error) async -> Action?),
      file: StaticString = #file,
      fileID: StaticString = #fileID,
      line: UInt = #line
    ) -> Self {
        ...
    }

which would require the usage to look like:

return .task {
    try someThrowingFunction()
    return .successfulAction
} catch: {
    return nil
}

That would make it even more clear that you are intentionally not returning an Action and you understand the consequences (potential hang state in the UI).

[rcarver]

👍 I just converted a non-throwing run to a throwing run and didn't feel great that I was handling it correctly

.run { send in
    for try await response in await self.vesperTemplateClient.fetchTemplates() {
        await send(.fetchTemplatesResponse(.success(response)))
    }
} catch: { error, send in
    await send(.fetchTemplatesResponse(.failure(error)))
}

Somewhat related, I typically use the block initializer for TaskResult to handle errors instead of the run { } catch: {} form. Is there a similar pattern that can be used with AsyncThrowingStream?

await send(.promotionalOffersResponse(TaskResult {
    try await self.upgradesClient.promotionalOffers()
}))

[patch-benjamin]

❤️ Ya, I like that approach and it actually came up in our team discussion around this topic as the way we should handle similar situations.
It's not quite enough to appease my anxiety though... Whenever a problematic code path can be executed I have anxiety around that. I teach an intro to iOS development course as a side gig and I am always conscious of trying to make my code impossible for an entry level dev (or me on a bad day ;P) to break. So while TaksResult solves the immediate issue, it still leaves the vulnerability of a more junior dev (or someone less familiar with TCA) accidentally calling .task and not providing a catch block. That mistake would be very easy to overlook in a PR and likely wouldn't show up in manual testing or a basic unit test suite for the Reducer and the consequences for it could be pretty intense (a frozen app).

[mbrandonw]

Hi @patch-benjamin, thanks for starting this discussion!

I have a few questions for you:

• Do you know about the TaskResult style of performing single units of async work? We highly recommend using that pattern when needing to execute throwing work inside .task { }:

  return .task {
    await .response(TaskResult { try await self.apiClient.fetch() })
  }

  That makes it so that all data from the dependency is sent back, both success and failures.
• Currently if a non-CancellationError error is thrown in an effect we show a purple runtime warning in Xcode. Did that help track down the problem at all?
• Also do you have any tests written for that user path? If a non-CancellationError error is throw in an effect during tests then it causes a test failure.

We currently find that the balance of not requiring catch but making some noise during runtime and tests provides decent ergonomics, though of course not perfect. If the purple runtime warning isn't loud enough, there's a chance we could make it customizable so you could opt to have it breakpoint when the error is thrown, or even fatalError.

Our main concern with adopting something like catch: nil is that we don't see how it actually fixes the problem. People will still default to doing catch: nil because it's easy, and when a problem like this occurs it's not like having catch: nil helps you realize what went wrong. You still have to understand that an action wasn't sent for failure, which means state wasn't updated, and you have to work your way backwards to figure out why the failure wasn't sent.

[patch-benjamin]

whenever Swift gets typed throws

I can't wait for that day... do/catch/throw feels like the most out of date part of Swift IMO.

We actually tried non-throwing closures at the beginning

You and @mbrandonw mentioned CancellationError which I haven't had exposer to yet in TCA (and it's been a hot minute since I've dealt with Combine directly). Are you saying that if you cancel a task (where the operation is non-throwing), it will call the catch block on the task function @stephencelis ?
So this would be valid:

return .task {
    let value = await someNonThrowingFunction()
    return .success(value)
}, catch: {
    // Even though the operation doesn't throw, the task can be cancelled which is a thrown error.
    return .cancelled
}

[stephencelis]

@patch-benjamin CancellationError comes from the standard library (it shipped with modern Swift concurrency), and it's how all of Swift concurrency cancellation works.

[benlings]

Would there be any way of surfacing these uncaught errors through an API (e.g. a global uncaughtErrorHandler)? The use case I'm thinking of is if there are error conditions that only appear in a deployed app. I'd like to be able to report these to a web service, so we can investigate and fix problems.

[stephencelis]

@benlings We'd like to make runtime warnings a bit more customizable and instrumentable, which could provide a solution, but we haven't dug into the problem yet.

In the meantime it should possible for you to create your own Effect helper that calls run under the hood and invokes such a handler in the catch closure.

[benlings]

Thanks for the suggestion - I’ll take a look at that.