A confusing use case for task cancellation

[crayment]

I have written the following failing test that demonstrates some behaviour I found confusing. Was a bit of a gotcha on our code base.

In this sample I found it very confusing that delayedWork is never performed. And I'm not even sure the best way to modify this code or our patterns to avoid accidentally creating this type of situation. Would like to avoid the parent having intimate knowledge about how the child is implemented. Hopefully the inline comments and the code below make the issue clear.

In our case we have a list of cells representing files. You can upload files which puts a cell in a top section of the list. When the upload completes we remove the uploading cell from the top section and append a new cell to the bottom section representing the new file that now exists on the backend. For unimportant reasons we had a small delay between removing the uploading cell and appending the new file cell.

@available(iOS 16, macOS 13, tvOS 16, watchOS 9, *)
func testTaskCancellationDemo() async {
    struct Parent: ReducerProtocol {
        struct State: Equatable {
            var count = 0
            var child: Child.State?
        }
        enum Action: Equatable {
            case child(Child.Action)
            case delayedWork
        }
        
        @Dependency(\.continuousClock) var clock
        
        var body: some ReducerProtocol<State, Action> {
            Reduce { state, action in
                switch action {
                case .child(.hook):
                    state.child = nil // Cancels child task by tearing down child view in real app
                    return .task {
                        // A small delay on the child's hook before we take further action in the parent.
                        // This task ends up cancelled and delayedWork action never runs
                        try await clock.sleep(for: .seconds(1))
                        return .delayedWork
                    }
                case .child:
                    return .none
                case .delayedWork:
                    state.count += 1
                    return .none
                }
            }
            .ifLet(\.child, action: /Action.child) {
                Child()
            }
        }
    }
    struct Child: ReducerProtocol {
        struct State: Equatable {}
        enum Action: Equatable {
            case task // Tied to SwiftUI.View task
            case hook // A hook to inform the parent this child is complete, no longer needed
        }
        
        func reduce(into state: inout State, action: Action) -> EffectTask<Action> {
            switch action {
            case .task:
                // Imagine we are subscribing to something here. In our case it is file upload progress.
                return .run { send in
                    await send(.hook)
                }
            case .hook:
                return .none
            }
        }
    }
    
    let clock = TestClock()
    let store = TestStore(initialState: Parent.State(child: Child.State()), reducer: Parent()) {
        $0.continuousClock = clock
    }
    let childTask = await store.send(.child(.task))
    await store.receive(.child(.hook)) {
        $0.child = nil
    }
    await childTask.cancel() // Would be done by SwiftUI
    
    await clock.advance(by: .seconds(1))
    await store.receive(.delayedWork) { // This is never received. Should it be?
        $0.count = 1
    }
}

[tgrapperon]

Hey @crayment!
I agree that this behavior is counter-intuitive. I think that your test isn't modelling exactly what's happening in a real-life, SwiftUI-backed, scenario. For example, if you rewrite your test like:

let clock = TestClock()
let store = TestStore(initialState: Parent.State(child: Child.State()), reducer: Parent()) {
  $0.continuousClock = clock
}
let childTask = Task { // This should inherit `@MainActor` from the test.
  await store.send(.child(.task)).finish() // We "classically" await for the send to finish.
}
// We need the above task to run a little. I'm megaYielding for this purpose.
// You need to import TCA as `@_spi(Internals) import ComposableArchitecture` 
// to access this helper.
await Task.megaYield()

await store.receive(.child(.hook)) {
  $0.child = nil
}
// This one is sync now, as this is a simple `Task`
childTask.cancel()  // Would be done by SwiftUI

await clock.advance(by: .seconds(1))
await store.receive(.delayedWork) {
  $0.count = 1
}
// We need to await `childTask` cancellation to go through and cancel/release its inner
// `send` for the test to succeed.
await Task.megaYield()

On my machine, the test passes, but this is very flakey.
I agree that this is cumbersome and error prone, and there are probably ways to improve the situation with some helpers. It is also possible that the current behavior of TestStoreTask is not quite right in this area.

[crayment]

Thanks for the response @tgrapperon!

Good point on adding the .finish() to the child task. That is definitely a difference between my test and what would actually happen in a real app. I'm not knowledgable enough to be sure where Task.megaYield is and isn't needed. I added a Task.megaYield after the cancel and before advancing the clock and the test failed again. I decided I would just get out of test land and back to a real minimal app to reproduce the issue. I created a repo here that does demonstrate the issue as I see it: https://github.com/crayment/tca-child-task-cancellation

Basically it shows a sheet then after a small delay a hook fires to "complete" the sheet which dismisses it and tries to increment the parent count after a delay. You will see the parent count never increments. Ideally I could find a way to tie the delayed work to the parent lifetime/task rather than the child's.

[tgrapperon]

Thanks for the repro. This is indeed counter-intuitive. You can check that if you don't await/finish in the child view and send a simple and sync viewStore.send(.task), it works as expected. If you make this child task cancellable with an ID that you share with the parent and if you cancel this effect at .childShowingChanged when the child state is nil, it also works as expected.

I think that something is maybe quite not 100% right with the way ViewStoreTask.finish() behaves. In my mind, .task or .run { send in send(.hook) } are already finished. The "problem" is that when you set the child state to nil and produce the ".delayedWork" task, the view is discarded right after, and your ViewStoreTask is cancelled while you're now awaiting to emit .delayedWork. But I wouldn't expect this task to be part of ViewStoreTask.finish() scope. The effect you spawned with the viewStore already emitted the .hook action and is now finished. Cancelling a finished task probably shouldn't cancel all the side effects it emitted along the way. Or if it should, we should probably reserve ViewStoreTask.finish for purely local effects and document this shortcoming.
/cc @mbrandonw, @stephencelis

[mbrandonw]

I looked into this briefly but haven't had the chance to discuss with Stephen. I'm traveling right now and have a broken wrist (😬), but will just put down some quick thoughts.

I do feel that this behavior is probably not correct. The ViewStoreTask returned from send has the special property that when cancelled it not only cancels the effect started from the action, but also any effects started from the effect's actions, and the effect's effect's actions, etc. We do that to tie as much of the feature's effects to the view lifecycle as possible.

But, it seems we are erroneously also tying the parent's effects lifecycle to the child view. And that definitely seems wrong. One option would be to stop trying to tie lifecycle of effects of effects to the view lifecycle. Maybe that was the wrong decision from the beginning. And instead our upcoming navigation tools should be able to deal with cancellation properly.

We'll look into this in more depth soon. Thanks for the repro @crayment!

[mbrandonw]

Looked into it a bit more and I don't think it's actually related to the "effects of effects" property I described above. I think it's happening simply because when you do viewStore.send, the ViewStoreTask you get back wraps the task that is returned from the very root of the scope chain. So, cancelling it cancels all effects for every scoped feature.

I don't have a proposed solution, just documenting my findings.

[crayment]

Appreciate the notes @mbrandonw! Speedy recovery on that wrist!

I am somewhat relieved to hear that this isn't just a known and intended consequence of how cancellation works now because I do think it can be pretty confusing. But I know the far reaching cancellation behaviour is also nice in other situations.

I have worked around this case in our app by removing the delay. Turns out it wasn't needed anymore. If we did need the delay I'm not sure what would be best though. Sharing cancel ID's between layers is something we have managed to avoid so far. Perhaps this all changes with the navigation stuff you have cooking though :)