Strange behaviour of nested `.concatenate`s

Related Projects Swift Composable Architecture
1

I'm running Composable Architecture 0.13.0 on iOS 14.2, and I'm experiencing an unexpected order of actions processed by the following reducer:

let reducer = Reducer<State, Action, Void> { state, action, _ in
    let result: Effect<Action, Never>

    switch action {
    case .a, .b, .c:
        result = .none
    case .d:
        result = .concatenate(
            .init(value: .a),
            .init(value: .b)
        )
    case .x:
        result = .concatenate(
            .init(value: .d),
            .init(value: .c)
        )
            // Received: .x, .d, .c, .a, .b
    }

    return result
}.debug()

When I send .x action to a view store with such a reducer, I receive .x, .d, .c, .a, .b sequence of events printed in console.

Note that for some reason .c precedes .a, .b, which is not what I would expect. My expectation is that .a, .b should precede .c, since .a, .b are concatenated into .d, which precedes .c.

If handling of .x case is rewritten to the following:

case .x:
    result = Publishers.Sequence(sequence: [.d, .c])
        .eraseToEffect()
        // Received: .x, .d, .c, .a, .b

I'm getting the same .x, .d, .c, .a, .b.

And only if the latter is amended to this:

case .x:
    result = Publishers.Sequence(sequence: [.d, .c])
        .receive(on: DispatchQueue.main)
        .eraseToEffect()
        // Received: .x, .d, .a, .b, .c

I'm getting the expected .x, .d, .a, .b, .c.

Is there any explanation of such behavior?

1 Like
2

This is the expected behaviour - if an effect publishes an action while a synchronous effect is running, the action will not be dispatched to the store until the effect has completed.

My expectation is that .a, .b should precede .c , since .a, .b are concatenated into .d , which precedes .c .

That's not what is happening - .a and .b are not concatenated into .d - when .d is sent to the store as a result of the .x effect, it will return a new effect that is completely independent of the effect returned by .x.

This new effect will not begin execution until the current .x effect has finished. The .x effect is synchronous and will complete after it has sent .d and .c to the store and at that point the store will pick up the next effect in its buffer (the .d effect) and execute that.

Using .receive(on:) on the .x effect turns it into an asynchronous effect, i.e. an effect that does not complete immediately upon execution - this allows the .d effect to start before the .x effect has finished.

You can see why this works by looking at how the store executes effects:

If the effect is synchronous the effect's value (the next action) will be received while isProcessingEffects is true which causes the action to be appended to the synchronousActionsToSend buffer, the effect then completes and then that action will get handled on the next iteration of the while loop.

If the effect is asynchronous then the effect's value will be received while isProcessingEffects is false and the action will be dispatched immediately.

If you want to run the actions in the order you expect, rather than using .receive(on:) you would be better off concatenting .init(value: .c) to the .d effect instead of the .x effect.

3

It does seem like there is a ordering bug here :grimacing:. @lukeredpath is correct that it is expected with how the code is written today, but we think the ordering logic should probably be beefed up a bit so that the order @SiarheiBarysenka mentions is preserved. Preventing actions from being sent just because a synchronous, concatenated effect is still running doesn't seem like the right choice.

We even have a test in the test suite that asserts on how these synchronous effect actions are ordered. It was written a long time ago (before open sourcing), and when I look at it now it looks to be in the wrong order for what I expect:

One easy fix is to just call self.send recursively inside receiveValue to ensure the order of the actions is correct:

        receiveValue: { [weak self] action in
-          if isProcessingEffects {
-            self?.synchronousActionsToSend.append(action)
-          } else {
            self?.send(action)
-          }
        }

However we specifically moved away from this long ago due to a stack overflow problem of sending lots of actions into the store.

@stephencelis and I took a moment to try to find another fix, but the solution hasn't come to us quite yet. We pushed a branch with two failing tests. This could be a good starting point if anyone wants to try to fix it, otherwise we'll try to devote some time to this in the coming days.

2 Likes
4

Hmm, so for me the current behaviour makes more sense and I wouldn’t want to see it change. It would also be a pretty big change that could break a lot of things for people.

I’m currently maintaining a ReSwift middleware that emulates the effects system from TCA and my original implementation did actually work recursively. For the same reasons as TCA, I backported the current TCA buffered implementation and moved away from recursive dispatch.

I’d rather see improved documentation about effect ordering to clarify this behaviour rather than a change to the ordering. I think it’s a lot easier to reason with effects being executed in a sequential queue rather than recursively.

It makes perfect sense to me that effects are executed in the order they are triggered. I’m struggling to see how this can be changed without reverting to the recursive implementation but then you’d be back to risking a stack overflow.

As people seem to have different expectations of how effects should be ordered, maybe the compromise here is to make the dispatch strategy configurable - sequential or recursive. The latter would come with the stack overflow caveat but if you’re confident you won’t run in to it then you can opt in to that behaviour.

1 Like
6

Ok, I dug a little deeper into the original design decisions made for Store.send and I've re-convinced myself that the current behavior is the correct :sweat_smile:

This was originally brought up by @pteasima before the library had been officially open sourced, and he came up with an illuminating example. Here's a version of it (this is also essentially what is in the test I linked above):

func testOrder() {
  enum AppAction: Equatable {
    case `init`, step1, step2
  }

  var result: [Int] = []

  let reducer = Reducer<Int, AppAction, Void> { _, action, _ in
    switch action {
    case .`init`:
      return .concatenate(
        .init(value: .step1),
        .fireAndForget { result.append(1) }
      )

    case .step1:
      return .concatenate(
        .init(value: .step2),
        .fireAndForget { result.append(2) }
      )

    case .step2:
      return .fireAndForget { result.append(3) }
    }
  }

  let store = Store(initialState: 0, reducer: reducer, environment: ())
  store.send(.`init`)
  XCTAssertEqual(result, [1, 2, 3])
}

With TCA today, the assertion result == [1, 2, 3] passes, and if we change it to immediately sending actions recursively rather than queuing them, the order would be result == [3, 2, 1].

The reason I find the [1, 2, 3] more correct is that it allows us to consider the work happening in the case .init in isolation. We can see that an effect will be returned that fires .a back into the system, and that then a .fireAndForget effect will be executed to mutate the result array. By queuing whatever actions .step1 triggers after .init's actions we can understand everything the reducer will do just by looking at what happens in .init.

If we change the behavior so that .step1's actions are enqueued before .init's then we will have to consider what all the later actions may do in order to understand what .init does in its entirety.

This probably does warrant some updates to the documentation because there is a lot of subtly not only with this synchronous action order stuff, but also re-entrancy of actions.

2 Likes
7

Thanks @lukeredpath and @mbrandonw for sharing your explanations and code examples!

Indeed, this behavior is expected in the axiomatics of sequential execution order. However, I can't really say it makes the code more intuitive, and I can't convince myself that it should be a default way of processing actions.

I'm wondering if sequential execution (implemented by default in TCA) is the best trade-off for solving stack overflow issues compared to other challenges it brings – what do you think? While stack overflow is indeed a problem, why not to let the author of the code to solve it manually, given that a trivial solution would be turning an effect into asynchronous one in place as far as I can see? Instead, TCA does this somewhat implicitly, and that's not always needed and expected.

I found it counter-intuitive that the 2-nd and 3-rd code examples from my initial post (namely, after adding .receive(on: DispatchQueue.main)) should indeed result in a reversed order of processed actions. Of course, sequential execution explains that, but to me it makes the reasoning about the program way harder.

Also, to the latest example that @mbrandonw shared, I found it quite weird that one may swap the order of operands within each .concatenate call in the reducer from this example, and nothing will change as a result. With recursive execution, one will get different results from these permutations, which I found to be a clear and self-explanatory outcome.

As @lukeredpath suggested, changing this to recursive order would be a super-breaking change indeed, so I also agree that making the execution strategy configurable may improve things for those who expect the recursive execution.

What do you all think?

1 Like