Unable to cancel an EffectTask before it's being run

[Thomvis]

Description

If a reducer, in response to an action, emits both an EffectTask.task with cancellable on it and an EffectTask.cancel (in that order) for the same id, the task is not cancelled.

Checklist

• I have determined whether this bug is also reproducible in a vanilla SwiftUI project.
• If possible, I've reproduced the issue using the main branch of this package.
• This issue hasn't been addressed in an existing GitHub issue or discussion.

Expected behavior

I would expect the task not to start because it was emitted and cancelled right away

Actual behavior

The task runs and finished

Steps to reproduce

The following example demonstrates the issue. In it, the .triggerIssue action emits two effects. The first is a longer running task that can be cancelled, the second is a cancel effect that I expect to cancel that very same long running task. I would expect that no .increment action is emitted because the task would be cancelled while sleeping. In reality, the .increment action is emitted.

In real world use, these effects would obviously not be right next to each other, but the issue remains the same.

@MainActor
func testImmediateCancellationReduced() async {
  struct Example: ReducerProtocol {
    struct State: Equatable {
      var count: Int
    }

    enum Action: Equatable {
      case triggerIssue
      case increment
    }

    func reduce(into state: inout State, action: Action) -> EffectTask<Action> {
      switch action {
      case .triggerIssue:
        return .concatenate([
          .task {
            try await Task.sleep(for: .seconds(1))
            return .increment
          }
          .cancellable(id: "1"),
          .cancel(id: "1")
        ])
      case .increment:
        state.count += 1
        return .none
      }
    }
  }

  let store = TestStore(initialState: Example.State(count: 0), reducer: Example())

  await store.send(.triggerIssue)

  await store.finish(timeout: .seconds(1))
}

The Composable Architecture version information

0.49.2

Destination operating system

iOS 16

Xcode version information

Version 14.1 (14B47b)

Swift Compiler version information

swift-driver version: 1.62.15 Apple Swift version 5.7.1 (swiftlang-5.7.1.135.3 clang-1400.0.29.51)
Target: arm64-apple-macosx12.0

[mbrandonw]

Hi @Thomvis, thanks for the detailed report!

Unless I'm missing something, this does behave how I would expect. You are concatenating the effects, and so the .cancel(id:) effect doesn't even start up until the cancellable effect finishes. Did you mean to do a merge?

But also, even if you do a merge it may not behave how you expect. This is because the .task effect incurs a thread hop due to switching to an asynchronous context, and the .cancel(id:) happens immediately and synchronously, so technically there's nothing to cancel at that point.

This is due to the fact that when we more fully embraced async tools in TCA we started incurring some context switches that were not there previously with Combine. We may be able to clean up some of those though.

All that to say, I don't have an immediate solution right now. We would have to look into making .cancellable do more of its work synchronously, and we will try to find time to investigate that.

[mbrandonw]

Just to drive home the difference between Combine and Swift concurrency, the following effect will operate how you want:

return .merge(
  .task { false }
    .eraseToEffect()
    .cancellable(id: 1),
  .cancel(id: 1)
)

This is because although .task { false } operates in the Swift concurrency world, once we hit it with eraseToEffect it becomes a regular Combine publisher, and in that world effects start up synchronously and immediately. So erasing your effects might be a way to work around this until we have more time to look into things.

[Thomvis]

Unless I'm missing something, this does behave how I would expect. You are concatenating the effects, and so the .cancel(id:) effect doesn't even start up until the cancellable effect finishes. Did you mean to do a merge?

I would expect the task to start, but be cancelled almost immediately while it's awaiting the Task.sleep. If I delay the .cancel by 1 ms it works as intended. Only if I try to cancel it in the same runloop tick when the task was emitted the cancel ends up in limbo. (The task has no entry in _cancellationCancellables yet so the .cancel is a no-op.)

But also, even if you do a merge it may not behave how you expect. This is because the .task effect incurs a thread hop due to switching to an asynchronous context, and the .cancel(id:) happens immediately and synchronously, so technically there's nothing to cancel at that point.

I figured this out after some debugging. I think it would be nice if I wouldn't have to think about the task's overhead. (Especially since Compose-based effects behave differently.)

Just to drive home the difference between Combine and Swift concurrency, the following effect will operate how you want:

return .merge(
 .task { false }
   .eraseToEffect()
   .cancellable(id: 1),
 .cancel(id: 1)
)

This works as expected and is a good enough work-around for now, thanks!

[mbrandonw]

I would expect the task to start, but be cancelled almost immediately while it's awaiting the Task.sleep. If I delay the .cancel by 1 ms it works as intended. Only if I try to cancel it in the same runloop tick when the task was emitted the cancel ends up in limbo. (The task has no entry in _cancellationCancellables yet so the .cancel is a no-op.)

I'm sorry, I may be missing something obvious, but because you are using .concatenate the .cancel does not start at all until the .task { ... } has fully finished. It has no choice to cancel anything in flight because the cancellable thing has already finished.

I tried delaying the cancellation like you mentioned, but it still works the same way:

return .concatenate([
  .task {
    try await Task.sleep(nanoseconds: NSEC_PER_SEC)
    return .increment
  }
    .cancellable(id: "1"),

  .cancel(id: "1")
  .delay(for: 0.1, scheduler: DispatchQueue.main)
  .eraseToEffect()
])

[Thomvis]

I'm sorry, I may be missing something obvious, but because you are using .concatenate the .cancel does not start at all until the .task { ... } has fully finished. It has no choice to cancel anything in flight because the cancellable thing has already finished.

My bad. I hadn't realized concatenate waits for the task to finish (although I know about concatenate in the Rx context). Also it seems that some important details might have gotten lost when I formulated the minimal reproduction case. I'll spend more time to get to the core of the issue.

[Thomvis]

Here's my second try at the reproduction case. I got rid of the concatenate and now use the (old) synchronous send API because that more closely matches the use case where I encountered the issue. (There the events were triggered by a binding triggered by SwiftUI.)

Uncommenting the eraseToEffect() reliably fixes the issue, making it a good work-around.

func testImmediateCancellationReduced() {
  struct Example: ReducerProtocol {
    struct State: Equatable {
      var count: Int
    }

    enum Action: Equatable {
      case count(Int)
      case doCalculation(Int)
      case calculationDidFinish(Int)
    }

    func reduce(into state: inout State, action: Action) -> EffectTask<Action> {
      switch action {
      case .count(let c):
        let oldCount = state.count
        state.count = c
        return .merge([
          .cancel(id: "\(oldCount)"),
          .init(value: .doCalculation(c))
        ])
      case .doCalculation(let c):
        return .task {
          try await Task.sleep(for: .seconds(1))
          return .calculationDidFinish(c*100)
        }
//        .eraseToEffect()
        .cancellable(id: "\(c)")
      case .calculationDidFinish:
        return .none
      }
    }
  }

  let store = TestStore(initialState: Example.State(count: 0), reducer: Example())

  store.send(.count(1)) {
    $0.count = 1
  }
  store.receive(.doCalculation(1))
  store.send(.count(2)) {
    $0.count = 2
  }
  store.receive(.doCalculation(2))

  let e = expectation(description: "")
  Task {
    await store.receive(.calculationDidFinish(200), timeout: .seconds(2))
    e.fulfill()
  }

  waitForExpectations(timeout: 3)
}

[mbrandonw]

Ah yeah, ok that makes more sense. I think this is something we can fix while still staying in the concurrency world with no Combine. We have a note for it and will look at it soon.

As a quick aside, you may be interested in this article for an alternative to sharing reducer logic via actions. 🙂