Dispatch issue with middleware.

[gistya]

I have discovered a likely crash scenario. Please see below.

Store's dispatchFunction var gets assigned in ReSwift/CoreTypes/Store.swift ll. 69–79:

    self.dispatchFunction = middleware
        .reversed()
        .reduce({ [unowned self] action in
            return self._defaultDispatch(action: action)
        }) { dispatchFunction, middleware in
            // If the store get's deinitialized before the middleware is complete; drop
            // the action without dispatching.
            let dispatch: (Action) -> Void = { [weak self] in self?.dispatch($0) }
            let getState = { [weak self] in self?.state }
            return middleware(dispatch, getState)(dispatchFunction)
    }

However on line 76 we have:

let dispatch: DispatchFunction = { [weak self] in self?.dispatch($0) }

Now, self?.dispatch($0) in turn calls this function on ll. 146-148:

 open func dispatch(_ action: Action) {
     dispatchFunction(action)
 }

In turn, the Store's dispatchFunction function var gets called here. However, at this point, dispatchFunction may be nil because ll. 69–79 may not have completed its task of assigning a value to it yet, and it has no default value. See it's declaration on line 35:

public var dispatchFunction: DispatchFunction!

This may crash when the middleware array is not empty coming into the init function and one of them calls dispatch, since if the .reduce function runs, it may call Store's instance var dispatchFunction before it has been initialized.

EDIT: This will not always happen, but it can happen depending on how middleware is structured. There is also an infinite loop crash that's possible; see below.

[DivineDominion]

Since the order of type member declarations (methods and properties) doesn't matter, I presume you wonder where dispatchFunction is initialized to an actual value? Cmd+F to the rescue: https://github.com/ReSwift/ReSwift/blob/master/ReSwift/CoreTypes/Store.swift#L69

What is the exact issue you're facing, by the way?

[gistya]

@DivineDominion Evidently I did not make the original post clear enough. Also my line numbers were wrong because I was using the code inside the GitHubBrowser example, which differs slightly from the current commit here. I have edited it to clarify this issue and I updated the line numbers.

The exact issue is that this will always crash if middleware array is not empty and the .reduce function runs.

It might be solved by initializing Store's dispatchFunction property prior to the other assignmet, i.e. self.dispatchFunction = self._defaultDispatch prior to line 69. However, I don't know if that would cause other problems.

Side note: there is no need to return a value from the initial Result of the .reduce function on l. 72, because the type of Result is DispatchFunction (i.e. (Action)->Void).

[DivineDominion]

The thing is, line 76 will not actually get executed until you dispatch an action. Then the whole middleware collection is reduce'd and the inner dispatch reference is formed and the passed along. So this confusing loop of function calls is merely the preparation of a call, not the call itself. (To falsify your claim that it will crash:: Does it actually do crash in an app of yours?)

The superfluous return is a good catch! Do you want to open a PR?

[gistya]

The following code will reproduce the crash that I described. It depends on how the middleware is structured.

For instance, in StoreMiddlewareTests.swift, change the dispatchingMiddleware closure to the following:

let dispatchingMiddleware: Middleware<StateType> = { dispatch, getState in
    return { next in
        dispatch(SetValueStringAction("crash"))

        return { action in

            if var action = action as? SetValueAction {
                dispatch(SetValueStringAction("\(action.value)"))
            }

            return next(action)
        }
    }
}

This will reproduce the crash. Boom.

Also... sure, I'll open a pull request for the superfluous return.

[gistya]

OK, see pull request 274 for the superfluous return removal.

Concerning the crash mentioned above, that bears further discussion, and I'm interested in what your thoughts might be on the issue.

[gistya]

I should also note, there is also a scenario where Middleware can cause an infinite loop crash by calling its DispatchFunction argument, unless it is structured in a very specific way.

To see this in action, replace dispatchingMiddleware with this:

let dispatchingMiddleware: Middleware<StateType> = { dispatch, getState in
    return { next in
        return { action in
            dispatch(action)
        }
    }
}

Explanation

Under the current setup, we make at least two trips through any middleware that calls its DispatchFunction argument (as in testCanDispatch()). We will also make two trips through any middleware that preceded it in the array of middleware being reduced.

The only reason we just make two trips, instead of an infinity of trips, in the existing tests, is because the dispatchingMiddleware dispatches a new action of a different type than what it was given, and it does so inside a type check that prevents this new action from subsequently triggering another dispatch on the second trip through:

        if var action = action as? SetValueAction {
            dispatch(SetValueStringAction("\(action.value)"))
        } 

The first go-round, action comes in with type SetValueAction, so the typecast to SetValueAction succeeds (as it's a downcast from the Action protocol). However, then an entirely new Action is created of type SetValueStringAction, and dispatched. Therefore on the second go-round, the typecast to SetValueAction will fail, preventing an infinite loop crash.

Even the best-case scenario gets much worse the more dispatching middleware there are in the array:

Total Length of Middleware Array	3	4	5	6
Number of Dispatching Middlewares	3	4	5	6
Number of NonDispatching Middlewares	0	0	0	0
Total number of Middleware Runs	12	20	30	42

It appears to follow the pattern of increase, O(N(N+1)) or basically O(N^2), where N is the number of dispatching middleware in the array.

Increasing the number of non-dispatching middleware in the array has a different impact:

Total Length of Middleware Array	3	4	5	6
Number of Dispatching Middlewares	2	3	4	5
Number of NonDispatching Middlewares	1	1	1	1
Total number of Middleware Runs	9	16	25	36

Total Length of Middleware Array	3	4	5	6
Number of Dispatching Middlewares	1	2	3	4
Number of NonDispatching Middlewares	2	2	2	2
Total number of Middleware Runs	6	12	20	30

It appears here that the pattern of increase is O(N(N+1) + M(N+1)) where N is the number of dispatching middleware and M is the number of non-dispatching middleware. For example if there are 4 dispatching and 2 non-dispatching, the total number of middleware runs (collectively across all of them) is 4(4+1) + 2(4+1) = 30.

Summary

To summarize:

• middleware that calls a dispatch function always runs twice
• all middleware will also run multiple times if any dispatching middleware are in the array when Store is initialized
• the number of runs increases exponentially as more dispatching middleware are added
• middleware that dispatches will crash with an infinite loop unless it wraps its dispatch in a type check & mutates the type of action before dispatching

[DivineDominion]

Interesting! That sums up my feelings about scaling the extensive use of middlewares. But there are no alternatives I can think of that reduce overhead. Sure, the O(N^2) increase puts load on the call stack; the alternative I have been using involved enqueueing side effects into "pending" states, then dispatch completion actions to dequeue. That keeps the call stack flat, but increases coding overhead.

Maybe it can make sense to collect services that performs side effects into a collection and use 1 middleware to iterate over all of them. This keeps the call stack flatter but does not change the big-O complexity.

(I wrote about the code of this experimental approach today: http://cleancocoa.com/posts/2017/09/reswift-middleware-different-target/)

[gistya]

But should we be making two trips through some of these middleware? It seems like each one should only ever get called once.

[DivineDominion]

I wonder why not. Is it that in some Middleware cases you already know the next logical step is to start the chain of reducers? Then instead of dispatch, which exercises the whole stack, you'd write reduce(SetValueStringAction(...)) or something?

[gistya]

Sorry I have been swamped on a huge project at work. I will revisit this question with a fresh mind soon, but I would personally want middleware to simply pass through like a serial queue, where each middleware runs in the order it was added, and its result goes to the next middleware, etc. I think what I had seen in here looked like an “abuse of reduce” and something much simpler and possible to reason about would seem desirable. Just my 0.02. But I will take another look and get back to you.

[d4rkd3v1l]

Any news here?
We're currently facing really weird crashes, that seem to come from actions dispatched in our middlewares.
We started to heavily use middlewares, also for async stuff like network requests and then just dispatch the results as actions again. Could this be (part of) the problem?

[dani-mp]

I make heavy use of middlewares in my app (all side effects, in fact) and I have 0 crashes and never had a problem regarding this. Can you share an example of a crashing middleware?

[d4rkd3v1l]

Sorry for the late answer and unfortunately I didn't have the time to try to reproduce this with a demo app yet.

The only thing that currently "fixes" the issue is, to identify "problematic" actions that are dispatched in middlewares and dispatch them using DispatchQueue.main.async.
To me it looks like there may be some kind of recursion or timing issue happening, as indeed DispatchQueue.main.async kinda breaks the current cycle, and dispatches the action just afterwards. Don't know exactly how to describe it, but I hope you can get the point anyway.

Anyway, I don't like that kind of "fix" and really want to get down to the root cause of the problem.

@dani-mp How do you use the middlewares? Just some questions that come to my mind...

• Is it important to call next(action) before or after side effects? Or doesn't it really matter at all?
• Is it generally "legal" to dispatch stuff on ReSwiftInit already? Or should this really explicitly be used to "init" ReSwift?

I already created a question on SO, but it's really hard to provide the necessary information needed, as I can't share the whole app code (which in fact may be necessary). So maybe I really need to get a demo working, IDK.

Anyway here's the SO link: https://stackoverflow.com/questions/61793803/strange-memory-issues-thread-1-exc-bad-access-code-2-address-0x16d09aa00

[mjarvis]

Interesting.

Is it important to call next(action) before or after side effects? Or doesn't it really matter at all?

It does matter -- but not for any safety reasons. Calling next(action) moves along to the next middleware and eventually the reducers. Its up to you to decide if you want the reducers to be performed before or after your side-effects.

Is it generally "legal" to dispatch stuff on ReSwiftInit already? Or should this really explicitly be used to "init" ReSwift?

I've never tested this. ReSwiftInit is a side-effect of allowing nil state at initialization, and is used to allow the reducers to initialize the base state.
Your actions are currently being dispatched before the init occurs, while state is still nil. Theoretically this should be okay -- as the reducers should just create the state for the first time, but I've not looked into this deeper.
Without further testing, one workaround for this specific case would be calling next(action) before dispatching any additional actions, to allow for the state initialization to occur.
I would moreso recommend as a general pattern to avoid using ReSwiftInit as a trigger for other actions. If you want certain state set right at initialization, do so in the reducers. If you wish for some side-effect to occur at launch, use some other trigger for dispatching the action (IE application(_:didFinishLaunchingWithOptions:) or some similar delegate or notification)

[DivineDominion]

What do y'all think about making ReSwiftInit internal instead of public to discourage hooking into this? At least I don't see any particular reason that app devs should work with this action 🤔

[mjarvis]

I've needed reference to it a couple times -- for explicitly skipping middleware behaviour on that action. There may be a better way to resolve that need, though.

[d4rkd3v1l]

Ok, I'm pretty sure I finally got the issue.

We have a quite large data structure (according to MemoryLayout.size(ofValue: getState()) our state is ~11kb in size). Considering structs are value types and get copied around all the time and the main thread stack is "only" 512kb in size (Thread.main.stackSize), we probably run into a stack overflow (IDK how you call a complete stack overflow, as it's not a "normal" BOF?).

Using those "dispatch" workaround breaks the middleware "recursion" and should considerably lower the stack usage here. I would love to directly confirm this, but unfortunately I was not able to find a method to observe the actual stack load. If anyone knows how to achieve this, please let me know!

I could also reproduce this behaviour in a completely stripped down demo project, and therefore I'm quite sure now, this is it.

If you're interested, I can put it here on GitHub somewhere.
In the meantime I'll think about how to get our data structures down or how to relieve the stack. Maybe that dispatch stuff is really a solution to at least reason about ;-)

[dani-mp]

Yes, please. It'd be great to have a demo project to troubleshoot this. We should also try to find information in the Swift community and see if this is a Swift issue or something that we're doing wrong in ReSwift.

I'm still not sure what you mean with Using those "dispatch" workaround breaks the middleware "recursion". If there is any recursion it's probably because the same action is being dispatched without any guard in a middleware, otherwise, the number of middleware passes should be finite.

@DivineDominion about ReSwiftInit, I know that in the original Redux implementation they marked some internal actions with some weird prefix to make sure people don't use it. More info here: reduxjs/redux#186. We could do something like that. I would even go farther and change the way the reducers are auto-initialised. This is something that has always bothered me but never created an issue about it. I'll create one so we don't mix the two things here (sorry about that, @d4rkd3v1l).

[d4rkd3v1l]

It's not like an endless recursion, but as long as you dispatch actions in the middlewares over and over again (before actually getting out of it) it's indeed (as intended) running through the middlewares again and again which produces quite larges call stacks.

I think @gistya provided some very nice insights on this already here: #271 (comment)

From my understanding (I may be wrong) stacks work like this: Each function call gets placed on the stack with its parameters and any "static" allocated memory (value types, or just references to reference types on the heap) and it's return address (doesn't actually matter how/what exactly goes on the stack). What matters is, when it gets popped off the stack again, and that's (in my understanding), when a function returns, which it only does in this case, when the "recursion" ends. DispatchQueue.main.async lets the "outer function" return and just afterwards executes the "dispatched thing/call".
Oh man, I'm really having a hard time explaining this, hopefully it's more clear what I mean. And hopefully someone can confirm or reject my theory :-)

As for ReSwiftInit, now I think this really doesn't have anything to do with my issue. But maybe it's a good idea to let devs know how/if they should use it or not. And maybe provide an alternative for init stuff (like you guys basically already did here).

[d4rkd3v1l]

Here's the demo project. It's somewhat similar to our app, but indeed not exactly the same. The data structure is a bit bigger in the demo project (adjustable in commenting in/out stuff in the dummy data structs, you'll see what I mean) and there is less (almost nothing) happening in terms of reducers etc. which in the end leads to similar results (crashes). For sake of simplicity it should suffice like this I guess.

https://github.com/d4rkd3v1l/ReSwift-StackOverflowDemo/blob/master/reswiftcrash/ReSwiftDemo.swift

[mjarvis]

I understand what you're saying in regards to call stack. Thats definitely a possibility given a large enough state and numerous middleware & reducers.

I do find it surprising that you've ran into this, though, I've worked with some fairly large ReSwift systems without encountering it.

Thanks for the demo project. Does this crash on simulator as well or just on device?

[d4rkd3v1l]

I do find it surprising that you've ran into this, though, I've worked with some fairly large ReSwift systems without encountering it.

We have some data structures in the state that are insanely large. 😬🙈

Thanks for the demo project. Does this crash on simulator as well or just on device?

Just on device

[d4rkd3v1l]

I "solved" it for now, by moving large structs (in fact all the sub-states) onto the heap, by wrapping them inside arrays. Now the AppState went down from ~11kb to 160b and everything runs smooth again. 🥳

Using @propertyWrappers, it feels at least like a partly elegant solution to me.
https://gist.github.com/d4rkd3v1l/ab582a7cafd3a8b8c164c8541a3eef96

[dani-mp]

That's super useful, @d4rkd3v1l, thanks! Is this a known problem in Swift? It seems to me that ReSwift is not doing anything wrong here, but I'm not sure if it can still do something to avoid this or if that would make sense.

[d4rkd3v1l]

I would not say ReSwift is doing anything wrong here. I just think it kinda facilitates running into such issues, due to it's very nature (large/many state structs).

But for me, a proper state handling, as ReSwift provides completely outweighs such issues. I rather live with such "hackarounds" (like the wrappers explained above) than living without proper state handling :-)

I just fell way too deep in love with ReSwift 😍(so my thoughts on this may not be objective at all 🙈🤣)

[DivineDominion]

The pattern of dispatching another action in a Middleware and the problems that stem from it (see @gistya's overview far at the top) became weird in a demo project I started recently, ReSwift-Mario. Here's the variant where a middleware would send another action as a response to a stimulus action, depending on some state: https://github.com/CleanCocoa/ReSwift-Mario/blob/b7fd7d4c1518de3ef66d0123067ddeecf2aae995/State/MovementMiddleware.swift

let movementMiddleware: Middleware<RootState> = { dispatch, getState in
    return { next in
        return { action in
            next(action)

            // Only move once per tick to have a constant movement speed, bound to the FPS.
            guard action is Tick else { return }
            guard let keysHeld = getState()?.keysHeld else { return }

            if keysHeld.contains(.left) {
                dispatch(Walking.left)
            } else if keysHeld.contains(.right) {
                dispatch(Walking.right)
            }
        }
    }
}

I found it works equally well and introduces far fewer passes of dispatching actions when the stimulus action's reducer takes care of this. For the example of walking, see https://github.com/CleanCocoa/ReSwift-Mario/blob/3f7e9b5fc56bbea31b6c52589ea82400e89d8c6f/State/Tick.swift

internal struct Tick: ReSwift.Action {
    init() {}
}

func reduce(_ tick: Tick, state: RootState) -> RootState {
    var state = state
    // ...
    state = walk(state)
    return state
}

let walkingSpeed: Double = 2

func walk(_ state: RootState) -> RootState {
    let offset: Double = {
        if state.keysHeld.contains(.left) {
            return -walkingSpeed
        } else if state.keysHeld.contains(.right) {
            return +walkingSpeed
        } else {
            return 0
        }
    }()
    var state = state
    state.x += offset
    return state
}

The bottom line: For my real-world apps, I am now reconsidering my approach to using Middleware, and if some of them could just as well be reducers that I didn't see at first.

[dani-mp]

Yeah, @DivineDominion, that's a good example. You want to use middleware to feed/put data from/to the outside world, via side effects.

If the information is just derived from a combination of current/next state and the action, it's better to use reducers because there is no side effect involved.