IfLetStore and Effect cancellation on view disappear

Related Projects Swift Composable Architecture
#2

Hey @darrarski, thanks for the detailed report!

It may seem like that assertFailure is a little strict, but it is indeed what we want, and without it you would be silently hiding a bug rather than loudly complaining of a programmer error :grimacing:

The bug is that when the dismiss button is tapped you immediately nil out the detail state, which causes the detail view to go away, which then causes the onDisappear to be invoked, which finally sends the .stopTimer action. However, that action will never be delivered to your reducer because the state has already been nil'd out, and so there is no longer anything to reducer on. That essentially means that without the assertionFailure the timer would be quietly running forever, sending actions to the store, and you wouldn't know unless you happen to have .debug() on the reducer to see everything that was coming in.

So, the assertion really is trying to catch a potential bug, but that's not to say that this is the best way to handle the situation. The optional higher-order reducer is good for optionally showing/hiding views based on state, but if you need to hook into the lifecycle methods from within that view, in particular onDisappear, then you are always going to run into that assertion.

So this has motivated us to find a version of optional that is lifecycle aware. We have sketched a version of the higher-order reducer here, along with a case study:

We want to spend a little more time with it and see if there is any more polish we can apply, but the results are promising so far. Want to give it a spin and see if it helps you?

3 Likes
SwiftUI drag to dismiss crash
How can The Composable Architecture really become composable?
#3

Oh, and I should also say, you can fix this without resorting to the experiment code I linked to and just stick with optional. You just have to make sure to cancel the timer before you nil out the detail state, and stop using onDisappear. The .lifecycle reducer extension I pasted above just helps automate that for you and helps keep all of detail's logic in one place.

1 Like
#4

Wow, thank you for your effort and quick replay! I really appreciate it :heart:

I will try to fix the issue in my demo project as you advised.

#5

That approach of encapsulating the lifecycle is quite interesting. In my case I'm not that interested on the lifecycle itself but just a way to close a screen, but still I can take some tricks from it.

I think the important thing here is understanding how critical the order is. I think got so accustomed to the combined(with other: Reducer) -> Reducer variant that I introduced that it was tricky to accept that I should reverse the order.

#6

@mbrandonw your solution works perfectly! This is exactly what I was looking for. Once again, thank you so much for all the effort you make with @stephencelis, helping others to build apps with composable architecture.

I pushed changes on a separate branch in demo repository. You can find it here: https://github.com/darrarski/tca-ifletstore-effect-cancellation-demo/tree/solution-lifecycle

Have a nice day!

#7

@darrarski that's awesome to hear, and thanks for being a beta tester of the method! :laughing:

We'll polish up the docs and case study for it in the coming days and push out a release soon!

#8

@mbrandonw I noticed a strange behavior when canceling effects from onDisappear. The underneath publisher is canceled, but not released from the memory. While I didn't notice such issues before, when canceling effects from regular actions, I can't be sure yet if the issue is not in my implementation (may be not related to TCA). I will post more info once I investigate the problem.

#9

@darrarski ok interesting. yeah please let us know if you have anymore details.

Are you seeing that the memory isn't released in the memory graph debugger?

#10

@mbrandonw I didn't have much time to work on this, but I've managed to prepare an example on a separate branch of my demo project: https://github.com/darrarski/tca-ifletstore-effect-cancellation-demo/tree/custom-timer

It uses your lifecycle-reducer concept, but instead to Effect.timer I am using a custom timer publisher I made especially for this example. I have added some ugly prints as well :slight_smile:

To reproduce the issue with publishers not being deallocated after cancel:

  1. Run the app in a simulator
  2. Tap "Present Detail" and "Dismiss Detail" couple of times
  3. Notice following output on the console:
^^^ CustomTimerPublisher.init (1)
^^^ CustomTimerSubscription.init (1)
^^^ CustomTimerSubscription.cancel
^^^ CustomTimerSubscription.deinit (0)

^^^ CustomTimerPublisher.init (2)
^^^ CustomTimerSubscription.init (1)
^^^ CustomTimerSubscription.cancel
^^^ CustomTimerSubscription.deinit (0)

^^^ CustomTimerPublisher.init (3)
^^^ CustomTimerSubscription.init (1)
^^^ CustomTimerSubscription.cancel
^^^ CustomTimerSubscription.deinit (0)

The number in parentheses is a count of instances in memory. It looks like the subscription is correctly canceled and deinitialized, but not the publisher.

I've also tried to use Xcode's memory debugger to investigate what holds the publisher instances in the memory, but unfortunately, I didn't found any valuable hints beside I confirmed the publishers were not deinitialized.

Perhaps there is something wrong with my custom publisher implementation, but I am not sure about it. It's rather simple.

Here is a link to all changes in the code I made for this example: https://github.com/darrarski/tca-ifletstore-effect-cancellation-demo/compare/solution-lifecycle...custom-timer?expand=1

UPDATE

It looks like wrapping the custom publisher in Deferred before converting it to Effect solves the issue with retained publishers. I pushed the changes here: https://github.com/darrarski/tca-ifletstore-effect-cancellation-demo/commit/7cdc0c6ffb0cae15d37b15be2bb25da8d0455fc2

I am still not sure what actually caused the problem. It could be connected with how Effect cancellables are stored, or perhaps my custom publisher implementation is missing something.

UPDATE 2

While my custom publishers are no longer retained (which seems to be ok), now the Deferred publishers are still in the memory (wrapped in PublisherBox), so it looks like the issue is not completely solved :frowning_face:

#11

Once again, thanks for the support! :heart:

I started to investigate the issue with retained publishers on a demo project from this thread. Eventually I decided to create a new thread and a new demo project especially for this issue I'm experiencing. I think it's not related to the lifecycle of a component, but somehow it only appears when using IfLetStore. More details in the following thread:

How can The Composable Architecture really become composable?
#12

Hi! I'm summoning this issue from the grave. So sorry :sweat_smile:

The context:
So the solution described above works fine if there is not much of composition. TCA is composable and that's what we want to embrace. My issue is how we can deal with optional reducers which have non-optional reducers.

I have published a fork of darraski's repo here https://github.com/hfossli-agens/tca-ifletstore-effect-cancellation-demo/tree/nested-views-problem

Basically the state tree is

AppState 
|--> DetailState?
     |--> time: Date
     |--> me: AvatarState
     |--> peer: AvatarState

The AvatarView wants to cancel any ongoing requests on onDisappear.

The problem
The current code examples for lifecycle is only directly applicable to optional reducers with no levels of view composition. E.g. my AvatarView inside DetailView crashes the reducer if it sends onDisappear actions because the state is nilled.

What I have tried:
I have had a bit of a hard time finding the best solution for this problem.

Notes:
The main goal is to cancel the ongoing effect. Note that one AvatarView shouldn't interfere with other AvatarViews. Using shared cancellation-ids between AvatarViews is a no-go. The AvatarView might be visible other places. We are passing the cancellation-id to the AvatarView via AvatarEnvironment. If we only had access to the environment via the ViewStore we could actually cancel without reducing.

Anyone else share my concerns and face similar problems?

1 Like
How can The Composable Architecture really become composable?
#13

Pragmatic approach

Considering there can be multiple DetailView and AvatarView this pragmatic solution is viable

struct GenericCancellationId: Hashable {
    var parent: AnyHashable?
    var current: AnyHashable
}

struct DetailEnvironment {
    var cancellationId: AnyHashable
    
    var timerID: AnyHashable {
        struct TimerID: Hashable {}
        return GenericCancellationId(parent: cancellationId, current: TimerID())
    }
    
    var meEnv: AvatarEnvironment {
        struct MeID: Hashable {}
        return AvatarEnvironment(
            cancellationId: GenericCancellationId(parent: cancellationId, current: MeID())
        )
    }
    
    var peerEnv: AvatarEnvironment {
        struct PeerID: Hashable {}
        return AvatarEnvironment(
            cancellationId: GenericCancellationId(parent: cancellationId, current: PeerID())
        )
    }
}

.lifecycle(onAppear: { env in
    Effect.timer(id: env.timerID, every: 1, tolerance: .zero, on: DispatchQueue.main)
        .map { _ in DetailAction.timerTicked }
}, onDisappear: { env in
    return Effect.concatenate(
        .cancel(id: env.timerID),
        .cancel(id: env.meEnv.cancellationId),
        .cancel(id: env.peerEnv.cancellationId)
    )
})

See rest of code here https://github.com/hfossli-agens/tca-ifletstore-effect-cancellation-demo/compare/nested-views-problem...nested-views-cancelling-in-parent

This handles the consideration of having multiple instances of DetailView and AvatarView at the same time. It is not ideal for several reasons

  • The developer needs to remember to explicitly cancel all underlying effects
  • The developer needs to be passed a cancellation id and combine that with its own cancellation id's if there are multiple independent async tasks
  • The views and reducers needs some parent view to handle the onDisappear in case it is being used by some optional reducer

Looking forward to hear your thoughts on the subject. :blush:

#14

The easiest solution would obviously be something like this

struct AvatarView: View {
    let store: Store<AvatarState, AvatarAction>
    
    var body: some View {
        WithViewStore(store) { viewStore in
            VStack(spacing: 16) {
                ...
            }.onAppear {
                viewStore.send(.onAppear)
            }.onDisappear {
                Effect.cancel(id: viewStore.environment.cancellationId)
            }
        }
    }
}

But I see one problem with making environment available on the ViewStore – higher order reducers may use use state to create environments. Example:

Reducer { state, action, env in 
   let dynamicEnv = Environment(searchEffect: API.search(query: state.searchQuery))
   return someReducer(&state, action, dynamicEnv)
}

That's probably something we shouldn't support anyway?

#15

Thanks for contributing @hfossli!

I had a similar issue to the one you are describing when implementing navigation stack in SwiftUI. Let's say you have a couple of screens on the stack. Each one is powered by a TCA's Store. You want to cancel all ongoing effects on a given screen when it is being dismissed. Sometimes you want to dismiss all screens and go back to the root view (first screen on the navigation stack). You will then encounter an issue because the states of your screens are being composed - each screen's state holds an optional state of the screen presented from it.

I managed to investigate the problem and found a way to compose the navigation stack state to avoid the mentioned issues with effect cancellation. This might not apply directly to your problem, but I hope it could help you find a solution.

You can check out my PoC project that contains a working example of SwiftUI navigation powered by Swift Composable Architecture, which includes effect cancellation and dismissing multiple screens at once.

#16

Thanks for replying Dariusz! Which branch should I be looking at? The one on the right here? https://github.com/darrarski/tca-swiftui-navigation-demo/compare/main...spike/tca-lifecycle

#17

Check out the main branch, as it contains the solution. I tried to use lifecycle reducer but I abandoned this idea and left the branch in case I would like to come back to it someday.

#18

To me it seems the solution on the main branch can be boiled down to this

In Screens/First.swift the ongoing effects is cancelled if there is any state

    case .second(.didDisappear),
         .second(.third(.didDisappear)):
      if state.isPresentingSecond == false, let second = state.second {
        state.second = nil
        return cancelSecondReducerEffects(state: second)
      }
      return .none

and this is done by cancelling the effect directly

func cancelSecondReducerEffects<T>(state: SecondState) -> Effect<T, Never> {
  Effect.cancel(id: state.fetchId).fireAndForget()
}

To me it seems like there's several issues here

  1. The issue of the discussion is that didDisappear might be called after state is nilled. Thus the cancellation is never happening as there is if-let for state being non-nil.
  2. If issue no 1 is correct there is also no way to retrieve the cancellation id from the state (it is too late - state is nilled)
  3. I don't see how you will not get a crash in TCA's optional func

image
image1922×1496 414 KB

Am I missing something?

#19
  1. I don't see how you will not get a crash in TCA's optional func

Let me explain how I manage to solve the issue. Presentation of SecondView in NavigationLink is not bound to the first screen child state (second property of FirstState) being nil or not. It is bound to an additional property in the parent state (isPresentingSecond boolean property in FirstState). This property drives the NavigationLink presentation. When the didDisappear action is sent from the second screen, its state is still there. In the first screen reducer, we check if the second screen should still be presented when it disappears (it could disappear because a user went back to the first screen, or presented the third screen from the second one).

The flow of presenting and dismissing the second screen from the first one looks like this:

  1. User taps on a button on the first screen that sends FirstAction.presentSecond(true).
  2. The first reducer handles the action and changes FistState.isPresentingSecond to true.
  3. NavigationLink on the first screen pushes the second screen on the stack.
  4. User taps on a back button on the second screen.
  5. NavigationLink on the first screen pops the second screen from the stack and sends FirstAction.presentSecond(false) action.
  6. The first reducer handles the action and changes FirstState.isPresentingSecond to false.
  7. The second screen disappears and sends SecondAction.didDisappear action.
  8. The first reducers handles the action, and because FirstState.isPresentingSecond is false, it sets FirstState.second to nil and cancel all ongoing effects started from the second reducer.

I hope it makes it clear.

#20

Thanks for taking the time to explain more detailed. Appreciated! The separation of var isPresentingSecond: Bool and var second: Second?-state is indeed a detail that makes the difference. I found out now that we do the same thing in our app for several screens.

What would happen if a list item no 9 where introduced where SecondState.avatar1.didDisappear and SecondState.avatar2.didDisappear is called after SecondAction.didDisappear? The state is already nilled out in step no 8. We could of course wait to nil out the state until there's a new onAppear-action, but that is also fragile and assumes a certain order of events. What do you think?

I do think this is adding quite a lot of cognitive load and gotchas. Getting experience with these gotchas takes time and costs money. It would really be beneficial if @mbrandonw or @stephencelis has a more integrated approach and looks at this holistically.

I believe some solution like

- myReducer.optional()
+ myReducer.optional(cancellationBag: env.cancellationBag)

would be beneficial. All pending effects could be cancelled at the moment the state is nilled out (or next time it tries to reduce an optional state for that cancellation-id/bag.

#21

I tried your mentioned approach here now:

The code is identical to your approach expect for one difference: it doesn't nil out the state in AppAction.detail.onDisappear because doing so would not allow for AppAction.detail.avatar1.onDisappear and AppAction.detail.avatar2.onDisappear to run. In other words this causes crash:

        case .detail(.onDisappear):
            state.detail = nil
            return .none

and this won't crash

        case .detail(.onDisappear):
            return .none

I am not 100% confident about this and I fear leaving the state "dangling" OR relying on the state to be "dangling" may cause new issues. Not sure. Would love to hear your thoughts on this.