EXC_BAD_ACCESS crash related to stack size.

[crayment]

Somewhere in the upgrade from 0.52.0 to 0.54.0 our app started crashing on device only with an EXC_BAD_ACCESS.
I initially posted about this in Slack here: https://pointfreecommunity.slack.com/archives/C04KQQ7NXHV/p1682538092501199

Note at that time I was able to fix the crash with a small change to the dependencies library, but that is no longer enough for our app.
Breaking our features into smaller private reducers fixes this crash. Like this:

var body: some ReducerProtocolOf<Self> {
        CombineReducers {  
            _Scopes()
            _IfLets()
            _IfLets2()
            core
        }
    }
}

private struct _Scopes: ReducerProtocol // ...

I'm wondering/hoping there is a better pattern here. Something the library can do to make these types of crashes harder to hit. I cam up with this change to the IfLetReducer, which does fix our crash.

With that change the size of our reducers Body types are greatly reduced, which I believe is what contributes to resolving the crash.

func testReducerBodySize() {
    // Before any changes
    XCTAssertEqual(MemoryLayout<AppFeature.Body>.size, 3872)
    XCTAssertEqual(MemoryLayout<Dashboard.Body>.size, 9920)
    XCTAssertEqual(MemoryLayout<ManagerDetails.Body>.size, 13784)
    XCTAssertEqual(MemoryLayout<AssetDetails.Body>.size, 17504)
    
    // With _IfLetReducer.(parent|child) change
    XCTAssertEqual(MemoryLayout<AppFeature.Body>.size, 1128)
    XCTAssertEqual(MemoryLayout<Dashboard.Body>.size, 208)
    XCTAssertEqual(MemoryLayout<ManagerDetails.Body>.size, 2208)
    XCTAssertEqual(MemoryLayout<AssetDetails.Body>.size, 2032)
}

Of note: We already make careful use of a property wrapper to ensure child features State is stored on the heap, preventing our State size from growing too large, which was an issue for us in the past.

struct MyFeature: ReducerProtocol {
  struct State: Equatable {
    @StoredOnHeap var childFeature: ChildFeature.State?
  }
}

So I guess my questions are:

1. Are there trade-offs to forcing heap storage on reducers that serve to combine other reducers?
2. If there is no real downside, would this be an appropriate change to make to the library?
3. If the change is appropriate, are there any existing patterns doing this in the library that would be more appropriate than the simple change to store as an array?

[stephencelis]

Both PresentationState and PresentationAction store things on the heap, so I wonder if adopting the new navigation tools would help avoid the problem?

[crayment]

@stephencelis I worry it won't because of the way _PresentationReducer internally stores base and destination. We are working on adopting the new navigation tools and I can report back when we are done though!

[stephencelis]

Hm, have you found that reducers themselves are impacting the stack? They typically don't hold much (just other, nested reducers), while most of the stack issues we've seen involve large state structs. Can you share any reducer-related findings you have?

[crayment]

Well that's my theory yes. And I think the fact that my change to _IfLetReducer fixes the crash proves it in this case, but I'm not 100% confident.

It may be confusing in my post but with the change to _IfLetReducer to store parent/child on the heap it reduces the memory layout of the Body of our AssetDetails reducer from 17504 to 2032. Neither of these numbers are close to the stack size max though so I could be measuring the wrong thing. But that change to the _IfLetReducer does resolve the crash for us.

[stephencelis]

It'd be good to have a better understanding of what is taking up the majority of the stack. I can see improvements like this bringing the stack back down just enough to not overflow, but it still might be the case that 95% of the stack is from a large app state struct. I think the general worry of moving reducers on the heap storage is slowing down the processing of user actions in complex reducers.

[lukeredpath]

Just chiming in to say I ran into a similar experience after upgrading our app to TCA 0.58.2 (problem also exists on 1.2.0) and finishing migrating our app entirely over to the Reducer protocol.

Specifically, after ruling out any issues with the size of our state or actions, I tracked a stack overflow down to a chain of 8 onChange reducers in one of the reducers near the root of our hierarchy (our root "logged in" reducer). Removing these made the stack overflow go away.

I tried creating a copy of the _OnChangeReducer but having it store the base reducer in an array to try and move it onto the heap but that didn't fix the problem for me.

[lukeredpath]

On further investigation it doesn't seem to be a problem specifically with onChange, but more the number of chained higher order reducer functions on my core Reduce - I have 11, 2x ifLet, 8x onChange and 1x dependency, in that order. If I comment out 2 onChange calls, it stops crashing. Strangely, if I comment out only 1 of the ifLet calls it also stops crashing. If I comment out both ifLet calls, not only does it no crash, but I am able to add another 6 onChange calls without it crashing. ifLet seems to be the problem here. Or at least, the ifLet that works with @PresentationState.

[lukeredpath]

I tried the solution proposed in the original post (putting base and destination inside a local copy of _PresentationReducer and that did not fix the problem for me.

[lukeredpath]

Some additional info - if I changed the reducer that all of this calls were chained off of to an EmptyReducer that also got rid of the stack overflow. But extracting my core reducer logic into a separate reducer type, i.e. going from Reduce(core) (where core was a private func) to Core() didn't help.

[lukeredpath]

I may have narrowed this down to an issue in DEBUG (non-optimised) builds when you have a large action switch statement in your reducer: I was able to reproduce the crash at 17 cases. Fewer cases did not crash. No crashes at all in release builds. Could be related to https://forums.swift.org/t/need-help-to-invesigate-a-possible-stack-overflow-issue/63158/7