IfLetStore and Effect cancellation on view disappear

darrarski · 2020-07-07T22:04:49.421Z

@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.

mbrandonw · 2020-07-08T12:52:39.544Z

@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?

darrarski · 2020-07-08T14:22:16.935Z

@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: GitHub - darrarski/tca-ifletstore-effect-cancellation-demo at 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

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

Run the app in a simulator
Tap "Present Detail" and "Dismiss Detail" couple of times
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: Comparing solution-lifecycle...custom-timer · darrarski/tca-ifletstore-effect-cancellation-demo · GitHub

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: Wrap custom publisher in Deferred to fix the issue with retained publ… · darrarski/tca-ifletstore-effect-cancellation-demo@7cdc0c6 · GitHub

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

darrarski · 2020-07-08T21:25:18.008Z

mbrandonw:

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

Once again, thanks for the support!

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:

IfLetStore, Effect cancellation retains publisher Swift Composable Architecture

I noticed that when using cancellable effects inside IfLetStore view, the publishers of the effects are retained in the memory after the effect is canceled. To explain the issue in a reproducible way, I've created a demo project and I'm sharing it on GitHub: https://github.com/darrarski/tca-ifletstore-effect-cancellation-retains-publisher-demo I think the problem is related to IfLetStore because when not using it the publishers are correctly disposed from the memory after being canceled. Perh…

hfossli · 2020-12-10T11:04:26.655Z

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

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 GitHub - hfossli-agens/tca-ifletstore-effect-cancellation-demo at 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?

hfossli · 2020-12-10T12:47:02.510Z

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 Comparing nested-views-problem...nested-views-cancelling-in-parent · hfossli-agens/tca-ifletstore-effect-cancellation-demo · GitHub

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.

hfossli · 2020-12-10T13:02:43.697Z

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?

darrarski · 2020-12-10T13:07:23.486Z

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.

GitHub

GitHub - darrarski/tca-swiftui-navigation-demo: Demo project that shows how...

Demo project that shows how to implement navigation in SwiftUI iOS application using Swift Composable Architecture - GitHub - darrarski/tca-swiftui-navigation-demo: Demo project that shows how to i...

hfossli · 2020-12-10T13:12:36.736Z

Thanks for replying Dariusz! Which branch should I be looking at? The one on the right here? Comparing main...spike/tca-lifecycle · darrarski/tca-swiftui-navigation-demo · GitHub

darrarski · 2020-12-10T13:18:25.280Z

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.

hfossli · 2020-12-14T09:31:00.940Z

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

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.
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)
I don't see how you will not get a crash in TCA's optional func

image1922×1496 414 KB

Am I missing something?

darrarski · 2020-12-14T11:12:36.318Z

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:

User taps on a button on the first screen that sends FirstAction.presentSecond(true).
The first reducer handles the action and changes FistState.isPresentingSecond to true.
NavigationLink on the first screen pushes the second screen on the stack.
User taps on a back button on the second screen.
NavigationLink on the first screen pops the second screen from the stack and sends FirstAction.presentSecond(false) action.
The first reducer handles the action and changes FirstState.isPresentingSecond to false.
The second screen disappears and sends SecondAction.didDisappear action.
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.

hfossli · 2020-12-14T14:34:41.321Z

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.

hfossli · 2020-12-14T14:51:55.621Z

I tried your mentioned approach here now:

GitHub

Comparing...

Contribute to hfossli-agens/tca-ifletstore-effect-cancellation-demo development by creating an account on GitHub.

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.

darrarski · 2020-12-14T16:32:50.455Z

I think this could definitely be improved. I experimented with an additional state wrapper (with a dedicated reducer) that holds the optional state and performs some actions when the wrapped state is set to nil. Found out it's not trivial to implement, as many corner cases should be rethought and handled gracefully. Something like this already exists in the Swift Composable Architecture repository - Lifecycle reducer example. It could be a good starting point.

dre7 · 2020-12-16T10:09:34.244Z

The solution I came to use is somewhat a hybrid of what you've been discussing and an approach that @mbrandonw used in a TCA sample code.

Every composable feature alongside the reducer defines something like:

enum PermissionsTeardownToken: CaseIterable, Hashable {
  case locationManagerId
}

Which simply defines tokens for all long-running Effects for that reducer. So when parent to Permissions reducer decides to nil it's state out:

    case .stop:
      state.permissions = nil
      return .cancel(token: PermissionsTeardownToken.self)

Where cancel(token:) is a tiny extension on Effect:

extension Effect {
  static func cancel<T>(token: T.Type) -> Effect where T: CaseIterable, T: Hashable {
    .merge(token.allCases.map(Effect.cancel(id:)))
  }
}

hfossli · 2020-12-18T09:58:22.438Z

Interesting. I like the idea.

Doesn't this solution assume there's just one "Permissions"-view? What if you had multiple instances of the Permissions-view at the same time? I think it is important that there's a cancellation id / token passed down.

darrarski · 2020-12-18T10:14:08.242Z

Doesn't this solution assume there's just one "Permissions"-view? What if you had multiple instances of the Permissions-view at the same time? I think it is important that there's a cancellation id / token passed down.

Right! This is very important. Thanks for bringing it up here. For that reason, in my example project, I stored the effect id in the state struct. I don't think it's a perfect solution, but I couldn't find a better one.

hfossli · 2020-12-18T14:17:34.242Z

Yep

For that reason, in my example project, I stored the effect id in the state struct

I fear it is a bit scary to store it in the state as the state tree is optional and leaving the state "dangling around" for a little while is a solution that might cause new issues.

New thread started here How can The Composable Architecture really become composable? - #2 by hfossli. I thought it would be a good idea to summarise and tell the story of the problem step by step.

hfossli · 2021-01-13T11:25:06.696Z

There's an update over here How can The Composable Architecture really become composable?