IfLetStore, Effect cancellation retains publisher

Related Projects Swift Composable Architecture
1

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.

Perhaps there is something I'm not aware of, connected to Effect cancellation in TCA. I appreciate any hints or ideas!

2

Thanks for sharing! I don't think this has to do with IfLetStore, though, because if I comment it out I still see the leak:

//IfLetStore(self.store.scope(
//    state: \.timer,
//    action: AppAction.timer
//)) { timerStore in
//    TimerView(store: timerStore)
//}
TimerView(
    store: self.store
        .scope(state: \.timer!, action: AppAction.timer)
)

Please let us know if you track down the leak! We've found a few leaks in vanilla Combine that have required workarounds. For instance:

3

You can try changing the root view from AppView to TimerView in SceneDelegate:

let view = TimerView(store: .init(
    initialState: .init(),
    reducer: timerReducer,
    environment: TimerEnvironment(
        mainQueue: AnyScheduler(DispatchQueue.main)
    )
))
window?.rootViewController = UIHostingController(rootView: view)
window?.makeKeyAndVisible()

I didn't notice such leaks when using TimerView directly, not as a "child" of AppView. Perhaps the issue is connected to scoping the store?

4

We dug into this a bit more and think we figured it out. If you swap out your custom timer publisher for Effect.timer we don't seem to get a memory leak at all:

-return CustomTimerPublisher(scheduler: environment.mainQueue)
+return Effect.timer(id: TimerEffectId(), every: 1, on: environment.mainQueue)
     .map { _ in TimerAction.tick }
     .eraseToEffect()
-    .cancellable(id: TimerEffectId(), cancelInFlight: true)

We then looked at your custom publisher and it uses Effect.timer under the hood but passes an empty string as a cancellation ID: tca-ifletstore-effect-cancellation-retains-publisher-demo/CustomTimer.swift at d661b23e57c3b1e111d99571abe6693c873ffdc3 · darrarski/tca-ifletstore-effect-cancellation-retains-publisher-demo · GitHub

Effect.timer(id: "", every: 1, tolerance: 0, on: scheduler)

Long-living effects are retained by cancellation ID, so if a custom publisher does some cancellation logic under the hood you need to forward on any cancel tokens on accordingly. If you want to control a timer-based effect outside of cancellation, you can use Publishers.Timer, defined in the combine-schedulers package that swift-composable-architecture uses: combine-schedulers/Timer.swift at ff42ec9061d864de7982162011321d3df5080c10 · pointfreeco/combine-schedulers · GitHub

5

Thanks for the reply, Stephen! You are totally right, I made a mistake and used Effect.timer inside my custom timer implementation. However, I think it wasn't a source of the problem. I replaced Effect.timer with Swift.Timer so my publisher no longer depends on TCA. The issue with publishers being retained after cancellation is still there :slightly_frowning_face:

You can check it out: GitHub - darrarski/tca-ifletstore-effect-cancellation-retains-publisher-demo at 20c5459edbf27f61c1a84d0edf8b638ff478914b

Here is a direct link to a single commit with the changes mentioned above: Replace Effect.timer with Swift.Timer in custom timer implementation · darrarski/tca-ifletstore-effect-cancellation-retains-publisher-demo@20c5459 · GitHub

6

@darrarski Is there a reason that you can't use Effect.timer directly, which doesn't exhibit this problem? If you can determine that you've found a bug that is definitely library-related we'd love to fix it, but right now the custom publisher adds enough complexity that it's difficult to troubleshoot if this is a bug with the Composable Architecture, Combine, the custom publisher, or something else. Do you think it's possible to reduce the issue to a failing unit test that can be PR'd?

We already have a couple of test suites going for memory management and cancellation:

7

The timer itself is not an issue here :slight_smile: I experienced the memory retain problem while working on another project, where I create a long-time-running effect from CoreData fetch request publisher. I noticed the issue is reproducible with any custom publisher I made. I am still not sure if the problem is caused by the TCA or my custom publishers.

Thanks for the advise, I will try to create a PR with failing unit test in TCA repository :+1:

8

Hey @stephencelis, I've created a PR on TCA repo that reproduces the issue. You can check it out here:

9

Good news. I was able to narrow the scope in my demo project and locate a single operator that, when used, is causing publishers to be retained. It looks like the problem might be in a place I didn't look at before.

Here is the simplified version of my demo app: GitHub - darrarski/tca-ifletstore-effect-cancellation-retains-publisher-demo at 3b589720170b574088c2cca51c413f8c3c148149

And here is the only change I made, after which publishers are no longer retained:

-let appReducer: Reducer<AppState, AppAction, AppEnvironment> = .combine(
+let appReducer: Reducer<AppState, AppAction, AppEnvironment> =
     timerReducer.pullback(
         state: \.timer,
         action: /AppAction.timer,
         environment: \.timerEnvironment
     )
-)

It looks like the issue is not connected to IfLetStore or .optional operator on a Reducer. It's strange that just using the Reducer.combine makes it retain publishers when effects are cancelled.

10

I am pleased to inform that the issue no longer occurs when using Xcode 12.1 (12A7403) with Apple Swift version 5.3 (swiftlang-1200.0.29.2 clang-1200.0.30.1) :tada:

2 Likes