ViewStore sometimes replays previous state values to subscribers

[adamastern]

Describe the bug

ViewStore publisher appears to sometimes replay its previous state value after publishing the latest, correct value.

To Reproduce

The scenario that produces this behavior is:

1. Send a ViewStore action which updates "Property A" on state.
2. Have a subscription on the same view store listening to "Property A". When the subscription receives the updated value from the .send above, synchronously dispatch another action which mutates "Property B".
3. Have a subscription on the view store listening to "Property B".
4. I would expect that the "Property B" subscriber receives two values. (1) for the initial value of B, and (2) the new value set in step 2.

Sometimes, the "Property B" subscriber receives a third, final update with the store's initial value for "Property B", after having received the value set in step 2.

The bug only repros ~50% of the time. The following test may need to run a number of times before this repros:

TcaWithTestCase.zip

  var cancellables: Set<AnyCancellable> = []

  func testOldStateReplaysInViewStoreSubscriber() {
    struct State: Equatable {
      var city: String
      var country: String
    }

    enum Action: Equatable {
      case updateCity(String)
      case updateCountry(String)
    }

    let reducer = Reducer<State, Action, Void> { state, action, _ in
      switch action {
      case let .updateCity(city):
        state.city = city
        return .none
      case let .updateCountry(country):
        state.country = country
        return .none
      }
    }

    let store = Store(
      initialState: .init(city: "New York", country: "USA"),
      reducer: reducer,
      environment: ()
    )
    let viewStore = ViewStore(store)

    viewStore.publisher.city
      .sink { city in
        // After we get the first update, let's update the country to "UK"
        if city == "London" {
          viewStore.send(.updateCountry("UK"))
        }
      }
      .store(in: &cancellables)

    var seenCountries = [String]()
    viewStore.publisher.country
      .sink { country in
        seenCountries.append(country)
      }
      .store(in: &cancellables)

    // Send the initial update to make city "London"
    viewStore.send(.updateCity("London"))

    // Test that we only see two updates:
    // 1) "USA"
    // 2) "UK"
    // Sometimes, we get a third update with "USA" again...

    XCTAssertEqual(seenCountries, ["USA", "UK"])
  }

Expected behavior

ViewStore shouldn't replay previous state value

Screenshots

n/a

Environment

• swift-composable-architecture version 0.23.0
• Xcode 12.5.1
• Swift 5.4
• OS: MacOS 11.12.3

[iampatbrown]

Thanks for providing a good example. I can confirm the same result on my end and am trying to get a better idea of the issue.

Is there a particular reason you're not putting the update country logic inside the reducer? My guess is that sending .updateCountry via sink is causing a race condition.

[krzysztofzablocki]

@iampatbrown note that if you also subscribe to the viewStore.publisher you'll see it always get 3 state updates and the difference between failed and successful run is the order being inconsistent.

UIKit / AppKit apps tend to use publisher observation a lot and this situations is likely to happen in bigger apps, causes weird to track down bugs.

[iampatbrown]

@krzysztofzablocki this is more to do with how combine works though right? You would probably have to send the action using DispatchQueue.main.async or use publisher.receive(on:)
For example the same issue occurs using the below

func testCombine() {
  var cancellables: Set<AnyCancellable> = []

  struct State: Equatable {
    var city: String
    var country: String
  }

  let state = CurrentValueSubject<State, Never>(.init(city: "New York", country: "USA"))

  state.map(\.city)
    .removeDuplicates()
    .filter { $0 == "London" }
    .sink { _ in state.value.country = "UK" }
    .store(in: &cancellables)

  var seenCountries = [String]()
  state.map(\.country)
    .removeDuplicates()
    .sink { seenCountries.append($0) }
    .store(in: &cancellables)

  state.value.city = "London"

  XCTAssertEqual(seenCountries, ["USA", "UK"])
}

[krzysztofzablocki]

when I was debugging TCA internals I didn't really notice anything wrong or inconsistent between failure / success cases on the side of library itself, so definitely feels like combine thing.

If there is no workaround for this kind of scenario probably worth adding a note in doc / sample

[mbrandonw]

There is some non-determinism in Combine that you are seeing here. It seems that the order of subscriptions isn't guaranteed. Running this in a playground you will see that sometimes it crashes and sometimes it doesn't:

var cancellables: Set<AnyCancellable> = []

let subject = CurrentValueSubject<Int, Never>(1)

var value = 0
subject.sink { value = $0 }
  .store(in: &cancellables)

subject.sink { assert($0 == value) }
  .store(in: &cancellables)

subject.send(1)

For this reason you probably should only do truly independent tasks inside .publisher.sink closures. So sending an action in one .sink { } that needs to be properly observed in another .sink { } is probably not going to work out well.

[mbrandonw]

I just opened this PR to improve the docs #699.

I do want to iterate @iampatbrown's point that it's probably not ideal to send actions from .sink. It does feel like that logic should be in the reducer.

However, that isn't what caused the weird behavior. It's entirely due to having multiple subscriptions on viewStore.publisher and assuming that they will be called in the order they were defined. That's the pattern you have to stay away from.

[diederich]

I played with this a little yesterday, and it looks like a small change could make this a little more deterministic.
The following is flipping the reset of isSending and applying the current value in Store's send, which I think gives back the expected ordering in case of those synchronous sends in ViewStore's sinks:

 defer {
        self.state.value = currentState
        self.isSending = false
        // if new actions were added synchronously
        // as part of state publisher updates,
        // process them now
        if !self.bufferedActions.isEmpty {
            send(self.bufferedActions.removeFirst())
        }
    }

Haven't thought much about any other effects this might introduce though.. :-/

[stephencelis]

@diederich Hm. I believe all synchronously buffered actions should be processed in this while loop:

swift-composable-architecture/Sources/ComposableArchitecture/Store.swift

Line 379 in bc5c510

while !self.bufferedActions.isEmpty {

Have you encountered a time where this wasn't the case? Do you think you could write up a failing test case?

[diederich]

This does fix the above failing / indeterministic use case.
Basically we're re-entering send, while processing the viewStore updates (triggered from the self.state.value in the defer)

[mbrandonw]

Here's another wrinkle... if you do two .sends in the .sink the test no longer passes:

viewStore.publisher.city
  .sink { city in
    if city == "London" {
      viewStore.send(.updateCountry("UK"))
      viewStore.send(.updateCountry("CA"))
    }
  }
  .store(in: &cancellables)

...

XCTAssertEqual(seenCountries, ["USA", "UK", "CA"]) 
// 🛑 XCTAssertEqual failed: ("["USA", "UK"]") is not equal to ("["USA", "UK", "CA"]")

So it seems processing the buffered actions immediately isn't a 100% fix to the problem.

We do see that there is a problem with sending actions from .sink, and we do know there is a use case for it (such as sending actions from a VC's viewDidLoad/viewWillAppear). It would be great if it could be fixed at the library level, but due to the non-determinism of Combine's subscription ordering we're not sure how.

It may just be that the recommendation is to always use .receive(on: DispatchQueue.main) if you need to send actions from a view store's subscription. Which means also doing it if you present controllers from a .sink 😕. This for example gets tests passing again, even with the two .sends:

viewStore.publisher.city
  .receive(on: DispatchQueue.main) // ⬅️
  .sink { city in
    if city == "London" {
      viewStore.send(.updateCountry("UK"))
      viewStore.send(.updateCountry("CA"))
    }
  }
  .store(in: &cancellables)

[diederich]

Ooops, sorry, I think this was because I grabbed the first action, and sending that, which messed up the order.

defer {
        self.state.value = currentState
        self.isSending = false
        // if new actions were added synchronously
        // as part of state publisher updates,
        // process them now
        if !self.bufferedActions.isEmpty {
          send(self.bufferedActions.removeLast())
        }
    }

fixes that I think?

[diederich]

Just updated the last snippet (it duplicated an action, but we obviously need to remove it from the bufferedActions before queuing it).
We're running with this patch and we haven't found any issues so far, but it does solve the out of order updates issue.
The last version also works with those "double" actions being sent from a sink.

I somehow like this, given the symmetry of it:

• we first set state to isSending, then we start processing actions
• when leaving the inner loop, we first process pending actions of any viewStore subscribers, then we flip back isSending