Avoid force-unwrapping cached value in ForEachStore

[ohitsdaniel]

We ran into a race-condition where force unwrapping the cached value lead to a crash. I'd like to suggest switching to a safer if-var pattern as see in the IfLetStore implementation to avoid potential crashes.

[stephencelis]

@ohitsdaniel Thanks for taking the time to PR!

Can you share a repro of the race condition?

We have to be very careful when making changes to ForEachStore due to subtle SwiftUI behaviors, and there've been a lot of PRs, issues, and discussions around it. We should probably have some kind of regression test app in place where we can ensure that changes don't impact performance.

I think we'll want to take a little time with this PR to ensure we don't introduce any performance regressions, but it would help to also have a reproducible crash, should we need to find an alternate fix.

[ohitsdaniel]

Thanks for the feedback! I can also see some issues with the content of the ForEach view becoming optional (an optional resolves to an EmptyView in a ViewBuilder, so it's still counted as an element and could lead to some empty rows).

I'll try to come up with a small example application that reproduces the crash. My assumption is that it has something to do with a .id() view modifier being applied to a view further out in the View hierarchy and that changing leads to the crash. Let's see, if I can reproduce it.

[stephencelis]

@ohitsdaniel This PR does unfortunately regress some behavior, where each row's view is initialized eagerly, even if the row is not in view. There was an issue/discussion about this, and a fix, but I wasn't able to quickly dig it up.

The easiest way to see the problem might be in the Todos project. On your branch:

1. Update AppState to be initialized with a bunch of todos:

   var todos: IdentifiedArrayOf<Todo> = .init(
     uniqueElements: (1...100).map { _ in Todo(id: .init()) }
   )

2. (Optional) Remove the .debug() line from appReducer to reduce noise.

3. Add the following initializer to TodoView:

   init(store: Store<Todo, TodoAction>) {
     print("TodoView.init")
     self.store = store
   }

4. Run the app.

You'll notice TodoView.init is printed 100 times, even though only 12-20 rows are rendered.

If you switch over to main and try again, TodoView.init will only be printed 12-20 times.

Initializing views should be very lightweight, so this could be a trade-off to take on if the problem is big enough. However, we also worry that incurring this amount of initialization could be quite bad for very large lists. Ideally there's another workaround to the problem.

[ohitsdaniel]

Thanks for this easy way to test if the new behaviour matches the old one. I think the reason for the crash mentioned in the feedback is ForEachStore's conformance to DynamicViewContent, but I decided not to investigate too much.

By introducing a EachState: Equatable constraint, we can use a plain ForEach in the content closure. ForEach captures the element in the ForEach content builder closure context, in case the element is removed and we run into the situation mentioned in the Feedback. This approach uses the 'cached' value to retrieve the ID and scopes the store using the cached ID and falls back to the element captured in the closure context.

If introducing a EachState: Equatable constraint on the init is too much of a breaking change from your perspective, I will try to think of something else. ;)

[ohitsdaniel]

We managed to reproduce the crash reliably.

To reproduce

Apply the diff below to the example TCA app, Todos.
Use XCode to run in simulator or on real device.
Press the Go Crazy button at the top. This will make the main thread busy for a couple seconds, creating and deleting many items many times.
Swipe down on the list of TODOs.
With the proposed fix, the Go Crazy button is still slow but the crash goes away.

Diff

diff --git a/Examples/Todos/Todos/Todos.swift b/Examples/Todos/Todos/Todos.swift
index 6945fa2..cca92aa 100644
--- a/Examples/Todos/Todos/Todos.swift
+++ b/Examples/Todos/Todos/Todos.swift
@@ -21,6 +21,9 @@ struct AppState: Equatable {
   }
 }
 
+let crazyNumberOfTodos = 100
+let crazyNumberOfTimes : UInt = 40
+
 enum AppAction: Equatable {
   case addTodoButtonTapped
   case clearCompletedButtonTapped
@@ -30,6 +33,7 @@ enum AppAction: Equatable {
   case move(IndexSet, Int)
   case sortCompletedTodos
   case todo(id: Todo.ID, action: TodoAction)
+  case goCrazy(howManyTimes: UInt)
 }
 
 struct AppEnvironment {
@@ -94,10 +98,22 @@ let appReducer = Reducer<AppState, AppAction, AppEnvironment>.combine(
 
     case .todo:
       return .none
+
+    case let .goCrazy(howManyTimes):
+      state.todos = []
+      if howManyTimes > 0 {
+        for _ in 1 ... crazyNumberOfTodos {
+          state.todos.insert(Todo(id: environment.uuid(), isComplete: true), at: 0)
+        }
+        return Effect(value: .goCrazy(howManyTimes: howManyTimes - 1))
+          .receive(on: environment.mainQueue)
+          .eraseToEffect()
+      } else {
+        return .none
+      }
     }
   }
 )
-.debug()
 
 struct AppView: View {
   let store: Store<AppState, AppAction>
@@ -147,10 +163,9 @@ struct AppView: View {
       .navigationBarItems(
         trailing: HStack(spacing: 20) {
           EditButton()
-          Button("Clear Completed") {
-            self.viewStore.send(.clearCompletedButtonTapped, animation: .default)
+          Button("Go Crazy") {
+            self.viewStore.send(.goCrazy(howManyTimes: crazyNumberOfTimes))
           }
-          .disabled(self.viewStore.isClearCompletedButtonDisabled)
           Button("Add Todo") { self.viewStore.send(.addTodoButtonTapped, animation: .default) }
         }
       )

[stephencelis]

@ohitsdaniel What if we have an outer fallback cache, as well? Does this fix things or cause any weird behavior?

    var outer: EachState?
    self.data = store.state.value
    self.content = {
      WithViewStore(store.scope(state: { $0.ids })) { viewStore in
        ForEach(viewStore.state, id: \.self) { id -> EachContent in
          // NB: We cache elements here to avoid a potential crash where SwiftUI may re-evaluate
          //     views for elements no longer in the collection.
          //
          // Feedback filed: https://gist.github.com/stephencelis/cdf85ae8dab437adc998fb0204ed9a6b
          var inner = store.state.value[id: id] ?? outer!
          outer = inner
          return content(
            store.scope(
              state: {
                inner = $0[id: id] ?? inner
                return inner
              },
              action: { (id, $0) }
            )
          )

[ohitsdaniel]

Hi Stephen,

Your fix works for my example! The race condition is reduced. But it still breaks reliably in a more complicated example.

With a class, it can happen that an IdentifiedArray is modified during view execution. This is of course not a good pattern, ideally devs only use structs in their states, but some types from libraries might be funny classes and outside of the control of developer. I guess TCA should be robust to that.

The repro makes lots of 1-element IdentifiedArrays, each todo inside a separate IdentifiedArray, and it tries to empty some of the inner arrays before the corresponding view can be evaluated.

I show the repro in two parts.

Repro diff 1/2 from upstream TCA: core stuff

diff --git a/Examples/Todos/Todos/Todo.swift b/Examples/Todos/Todos/Todo.swift
index 327b7a5d8..279e38363 100644
--- a/Examples/Todos/Todos/Todo.swift
+++ b/Examples/Todos/Todos/Todo.swift
@@ -2,6 +2,22 @@ import ComposableArchitecture
 import Foundation
 import SwiftUI
 
+class IdentifiedCell<T> : Equatable, Identifiable where T: Equatable {
+  public var id: UUID
+  public var value: T
+  public var pointer: IdentifiedCell<T>?
+
+  init(_ value: T, pointer: IdentifiedCell<T>? = .none) {
+    self.id = .init()
+    self.value = value
+    self.pointer = pointer
+  }
+
+  static func ==(lhs: IdentifiedCell<T>, rhs: IdentifiedCell<T>) -> Bool {
+      return lhs.id == rhs.id && lhs.value == rhs.value
+  }
+}
+
 struct Todo: Equatable, Identifiable {
   var description = ""
   let id: UUID
diff --git a/Examples/Todos/Todos/Todos.swift b/Examples/Todos/Todos/Todos.swift
index 6945fa294..bd2b618ac 100644
--- a/Examples/Todos/Todos/Todos.swift
+++ b/Examples/Todos/Todos/Todos.swift
@@ -7,10 +7,12 @@ enum Filter: LocalizedStringKey, CaseIterable, Hashable {
   case completed = "Completed"
 }
 
+typealias IdentifiedCellTodoArray = IdentifiedCell<IdentifiedArrayOf<Todo>>
+
 struct AppState: Equatable {
   var editMode: EditMode = .inactive
   var filter: Filter = .all
-  var todos: IdentifiedArrayOf<Todo> = []
+  var todos: IdentifiedArrayOf<IdentifiedCellTodoArray> = []
 
   var filteredTodos: IdentifiedArrayOf<Todo> {
     switch filter {
@@ -21,6 +23,9 @@ struct AppState: Equatable {
   }
 }
 
+let crazyNumberOfTodos = 100
+let crazyNumberOfTimes = 40
+
 enum AppAction: Equatable {
   case addTodoButtonTapped
   case clearCompletedButtonTapped
@@ -30,6 +35,7 @@ enum AppAction: Equatable {
   case move(IndexSet, Int)
   case sortCompletedTodos
   case todo(id: Todo.ID, action: TodoAction)
+  case goCrazy(howManyTimes: UInt)
 }
 
 struct AppEnvironment {
@@ -94,10 +100,25 @@ let appReducer = Reducer<AppState, AppAction, AppEnvironment>.combine(
 
     case .todo:
       return .none
+
+    case let .goCrazy(howManyTimes):
+      state.todos = []
+      if howManyTimes > 0 {
+        var next : IdentifiedCellTodoArray?
+        for i in 0 ..< crazyNumberOfTodos {
+          let todo = Todo(description: "\(howManyTimes) / \(i)", id: environment.uuid(), isComplete: true)
+          next = .init([todo], pointer: next)
+          state.todos.insert(next!, at: 0)
+        }
+        return Effect(value: .goCrazy(howManyTimes: howManyTimes - 1))
+          .receive(on: environment.mainQueue)
+          .eraseToEffect()
+      } else {
+        return .none
+      }
     }
   }
 )
-.debug()
 
 struct AppView: View {
   let store: Store<AppState, AppAction>
@@ -135,10 +156,19 @@ struct AppView: View {
         .padding(.horizontal)
 
         List {
-          ForEachStore(
-            self.store.scope(state: \.filteredTodos, action: AppAction.todo(id:action:)),
-            content: TodoView.init(store:)
-          )
+          ForEachStore(self.store.scope(
+              state: \.todos,
+              action: AppAction.todo(id:action:))) { firstStore in
+            WithViewStore(firstStore) { viewStore in
+              ForEachStore(firstStore.scope(
+                  state: \.value,
+                  action: TodoActionWrapper.Wrap(id:action:))) { secondStore in
+                // On every second TodoView, this quickly removes the next Todo
+                let _ = viewStore.state.pointer?.value = []
+                TodoView.init(store: secondStore)
+              }
+            }
+          }
           .onDelete { self.viewStore.send(.delete($0)) }
           .onMove { self.viewStore.send(.move($0, $1)) }
         }
@@ -147,10 +177,9 @@ struct AppView: View {
       .navigationBarItems(
         trailing: HStack(spacing: 20) {
           EditButton()
-          Button("Clear Completed") {
-            self.viewStore.send(.clearCompletedButtonTapped, animation: .default)
+          Button("Go Crazy") {
+            self.viewStore.send(.goCrazy(howManyTimes: crazyNumberOfTimes))
           }
-          .disabled(self.viewStore.isClearCompletedButtonDisabled)
           Button("Add Todo") { self.viewStore.send(.addTodoButtonTapped, animation: .default) }
         }
       )

Repro diff 2/2 from upstream TCA: support

diff --git a/Examples/Todos/Todos/Todos.swift b/Examples/Todos/Todos/Todos.swift
index bd2b618ac..5ccef637d 100644
--- a/Examples/Todos/Todos/Todos.swift
+++ b/Examples/Todos/Todos/Todos.swift
@@ -14,11 +14,16 @@ struct AppState: Equatable {
   var filter: Filter = .all
   var todos: IdentifiedArrayOf<IdentifiedCellTodoArray> = []
 
+  var flatTodos: IdentifiedArrayOf<Todo> {
+    let elements = self.todos.elements.flatMap { inner in inner.value.elements };
+    return IdentifiedArrayOf.init(uniqueElements: elements)
+  }
+
   var filteredTodos: IdentifiedArrayOf<Todo> {
     switch filter {
-    case .active: return self.todos.filter { !$0.isComplete }
-    case .all: return self.todos
-    case .completed: return self.todos.filter(\.isComplete)
+    case .active: return self.flatTodos.filter { !$0.isComplete }
+    case .all: return self.flatTodos
+    case .completed: return self.flatTodos.filter(\.isComplete)
     }
   }
 }
@@ -34,8 +39,12 @@ enum AppAction: Equatable {
   case filterPicked(Filter)
   case move(IndexSet, Int)
   case sortCompletedTodos
-  case todo(id: Todo.ID, action: TodoAction)
-  case goCrazy(howManyTimes: UInt)
+  case todo(id: Todo.ID, action: TodoActionWrapper)
+  case goCrazy(howManyTimes: Int)
+}
+
+enum TodoActionWrapper: Equatable {
+  case Wrap(id: Todo.ID, action: TodoAction)
 }
 
 struct AppEnvironment {
@@ -45,6 +54,10 @@ struct AppEnvironment {
 
 let appReducer = Reducer<AppState, AppAction, AppEnvironment>.combine(
   todoReducer.forEach(
+    state: \.value,
+    action: /TodoActionWrapper.Wrap(id:action:),
+    environment: { _ in TodoEnvironment() }
+  ).forEach(
     state: \.todos,
     action: /AppAction.todo(id:action:),
     environment: { _ in TodoEnvironment() }
@@ -52,11 +65,10 @@ let appReducer = Reducer<AppState, AppAction, AppEnvironment>.combine(
   Reducer { state, action, environment in
     switch action {
     case .addTodoButtonTapped:
-      state.todos.insert(Todo(id: environment.uuid()), at: 0)
+      state.todos.insert(.init([Todo(id: environment.uuid())]), at: 0)
       return .none
 
     case .clearCompletedButtonTapped:
-      state.todos.removeAll(where: \.isComplete)
       return .none
 
     case let .delete(indexSet):
@@ -90,14 +102,8 @@ let appReducer = Reducer<AppState, AppAction, AppEnvironment>.combine(
         .eraseToEffect()
 
     case .sortCompletedTodos:
-      state.todos.sort { $1.isComplete && !$0.isComplete }
       return .none
 
-    case .todo(id: _, action: .checkBoxToggled):
-      struct TodoCompletionId: Hashable {}
-      return Effect(value: .sortCompletedTodos)
-        .debounce(id: TodoCompletionId(), for: 1, scheduler: environment.mainQueue.animation())
-
     case .todo:
       return .none
 
@@ -137,7 +143,7 @@ struct AppView: View {
     init(state: AppState) {
       self.editMode = state.editMode
       self.filter = state.filter
-      self.isClearCompletedButtonDisabled = !state.todos.contains(where: \.isComplete)
+      self.isClearCompletedButtonDisabled = !state.flatTodos.contains(where: \.isComplete)
     }
   }
 
@@ -192,31 +198,33 @@ struct AppView: View {
   }
 }
 
-extension IdentifiedArray where ID == Todo.ID, Element == Todo {
-  static let mock: Self = [
-    Todo(
-      description: "Check Mail",
-      id: UUID(uuidString: "DEADBEEF-DEAD-BEEF-DEAD-BEEDDEADBEEF")!,
-      isComplete: false
-    ),
-    Todo(
-      description: "Buy Milk",
-      id: UUID(uuidString: "CAFEBEEF-CAFE-BEEF-CAFE-BEEFCAFEBEEF")!,
-      isComplete: false
-    ),
-    Todo(
-      description: "Call Mom",
-      id: UUID(uuidString: "D00DCAFE-D00D-CAFE-D00D-CAFED00DCAFE")!,
-      isComplete: true
-    ),
-  ]
-}
+let mock: [Todo] = [
+  Todo(
+    description: "Check Mail",
+    id: UUID(uuidString: "DEADBEEF-DEAD-BEEF-DEAD-BEEDDEADBEEF")!,
+    isComplete: false
+  ),
+  Todo(
+    description: "Buy Milk",
+    id: UUID(uuidString: "CAFEBEEF-CAFE-BEEF-CAFE-BEEFCAFEBEEF")!,
+    isComplete: false
+  ),
+  Todo(
+    description: "Call Mom",
+    id: UUID(uuidString: "D00DCAFE-D00D-CAFE-D00D-CAFED00DCAFE")!,
+    isComplete: true
+  ),
+]
 
 struct AppView_Previews: PreviewProvider {
+  static let todos = IdentifiedArrayOf.init(uniqueElements: mock.map {
+    todo in IdentifiedCell(IdentifiedArrayOf.init(uniqueElements: [todo]))
+  })
+
   static var previews: some View {
     AppView(
       store: Store(
-        initialState: AppState(todos: .mock),
+        initialState: AppState(todos: todos),
         reducer: appReducer,
         environment: AppEnvironment(
           mainQueue: .main,

[iblagajic]

Hey guys 👋

I am able to reproduce the crash on my side, but still trying to figure out a reasonably sized example I could provide here.

My app crashes when using a search bar to filter out a LazyVStack subview that's not visible at that time (is "beyond" the bottom of the stack).
However, it isn't easy to reproduce since it depends on some factors that seem unrelated at first (e.g. type of subviews in the LazyVStack).

Adding a small delay (10ms) before loading the new state fixes it, but ofc I'm not happy with that solution. 😀
My best guess so far is that it depends on rendering time, but still digging to find some concrete evidence.

If you have any tips how to proceed, I'd be thankful. 😇

[tgrapperon]

Hey @iblagajic!
I have a branch that reworks ForEachStore implementation. I think it should build in its current form. Would you mind giving it a spin to check if it helps? You only have to replace your TCA dependency by:

.package(url: "https://github.com/tgrapperon/swift-composable-architecture", branch: "generic-foreach"),

It should build without touching anything hopefully.

[iblagajic]

@tgrapperon thanks for the reply! 🙇

Unfortunately (or fortunately?), since I reset the cache to switch to your branch, I can't reproduce it on either branch anymore (generic-foreach or 0.44.1). I had one specific case that consistently crashed and now it's working fine on 0.44.1.
If I'm able to reproduce it again, I'll post here. 🤷

[tgrapperon]

@iblagajic It is also possible that you hit this crash if this is something fixed when building from a clean state.

[iblagajic]

@tgrapperon Thanks for the heads up.

Unfortunately, I was able to reproduce this crash again on 0.44.1 and this time I tried to check out your branch too. However, it still crashes (IterableContainer, line 43).

[tgrapperon]

@iblagajic. Yes, this is indeed the equivalent of the force-unwrap. It means that no data can be found for the given id, which shouldn't happen. Feel free to open a dedicated issue/discussion if you manage to extract a reproduction.

[stephencelis]

Sorry for sitting on this for so long, but just revisited and rebased and think it's a good change, thanks!