pointfreeco / swift-composable-architecture

A library for building applications in a consistent and understandable way, with composition, testing, and ergonomics in mind.
https://www.pointfree.co/collections/composable-architecture
MIT License
12.35k stars 1.44k forks source link

TCA holds on to state for longer than expected #3045

Closed pfandrade closed 5 months ago

pfandrade commented 5 months ago

Description

I'm finishing my first app with TCA and was trying to track down a memory leak. While doing so I found an unexpected behaviour in TCA:

If a sheet is presented and then dismissed, the associated feature state is not released after the dismiss. Only after the sheet is presented again is the state released. It seems the previous state is being held somewhere within TCA.

I've used the SyncUps example to confirm it's not something in my project. Here's a patch for the simple test I made:

diff --git a/Examples/SyncUps/SyncUps/SyncUpForm.swift b/Examples/SyncUps/SyncUps/SyncUpForm.swift
index 4e5e25e8d..b099926c6 100644
--- a/Examples/SyncUps/SyncUps/SyncUpForm.swift
+++ b/Examples/SyncUps/SyncUps/SyncUpForm.swift
@@ -8,7 +8,8 @@ struct SyncUpForm {
   struct State: Equatable, Sendable {
     var focus: Field? = .title
     var syncUp: SyncUp
-
+    var disposable = Disposable { print("DEINIT") }
+    
     init(focus: Field? = .title, syncUp: SyncUp) {
       self.focus = focus
       self.syncUp = syncUp
@@ -22,6 +23,8 @@ struct SyncUpForm {
       case attendee(Attendee.ID)
       case title
     }
+      
+      
   }

   enum Action: BindableAction, Equatable, Sendable {
@@ -136,3 +139,18 @@ extension Duration {
     )
   }
 }
+
+class Disposable: Equatable {
+    static func == (lhs: Disposable, rhs: Disposable) -> Bool {
+        lhs === rhs
+    }
+    
+    init(_ onDispose: @escaping () -> Void) {
+        self.onDispose = onDispose
+    }
+    var onDispose: () -> Void
+    
+    deinit {
+        onDispose()
+    }
+}

Checklist

Expected behavior

I would expect that "DEINIT" is printed when the SyncUpForm is dismissed.

Actual behavior

"DEINIT" is only printed after the second time the sheet is presented.

Steps to reproduce

  1. Apply the patch above
  2. Add a new sync up
  3. Dismiss it
  4. Confirm DEINIT is not printed

The Composable Architecture version information

5d73967c3ec8025626e07694c332aab77e6949a7

Destination operating system

iOS 17

Xcode version information

Version 15.3 (15E204a)

Swift Compiler version information

swift-driver version: 1.90.11.1 Apple Swift version 5.10 (swiftlang-5.10.0.13 clang-1500.3.9.4)
Target: arm64-apple-macosx14.0
stephencelis commented 5 months ago

@pfandrade This is a behavior of SwiftUI navigation: when a screen is presented and dismissed, often times the destination view and state are kept around in memory till the next presentation. You can take our vanilla SyncUps app that uses plain old observable models and add this to SyncUpFormModel and you'll see the same behavior:

deinit {
  print("DEINIT")
}

And the behavior is reproducible in much simpler apps, as well.

So I do not think you should depend on the lifetime of objects held in the view graph being tied to the lifetime of views on the screen, and instead you can depend on other, more predictable hooks.

Since this isn't a bug in TCA, and is just vanilla SwiftUI behavior, I'm going to convert this to a discussion.