WIP: Create RecursiveMutableProperty

[mluisbrown]

Create RecursiveMutableProperty where the sending of updates to observers is outside the scope of the synchronisation lock.

This is an attempt to avoid having to use DispatchQueue.main.async in order to avoid deadlocks when observers of state send actions to the store which recursively modify the state.

[mluisbrown]

It turns out that this approach won't work either. The problem is actually not in MutableProperty but in in Signal.Core which doesn't allow recursive sending of values: https://github.com/ReactiveCocoa/ReactiveSwift/blob/master/Sources/Signal.swift#L108

So it would seem it's not possible to solve this without changing ReactiveSwift itself 😕

[maximkrouk]

Would you like me to migrate the PR to ReactiveSwift itself? Or u can do it? c:

[maximkrouk]

Oh, it's probably more difficult than I thought 🤔

[mluisbrown]

I want to understand the motivation for not allowing recursive send of values in Signal. @andersio could you shed any light on that?

[maximkrouk]

Trying to understand it too, but still can't 🌚

[maximkrouk]

Maybe the reason is that if an observer sends event recursively, than the rest of observers will get the next event before the previous?

Pseudocode:

s: Signal = Observers[o1, o2(send2onReceive1), o3]
s.send(1) {
    o1(1)
    o2(1)
    o1(2)
    o2(2)
    o3(2)
    o3(1)
}

[maximkrouk]

OpenCombine.PassthroughSubject seems to be implemented in a similar manner https://github.com/OpenCombine/OpenCombine/blob/master/Sources/OpenCombine/PassthroughSubject.swift#L68

[andersio]

Recursive, synchronous properties do not have sensible behaviour. The general expectation is that property setters have immediate effect:

let property = MutableProperty(0)
property.value = 1
expect(property.value) == 1

and that ReactiveSwift guarantees that the latest value you are notified of is the same as Property.value, for the duration of the observer callout

property.producer.startWithValues { value in
    expect(value) == property.value

    someOtherWorkThatReads(property) // sees the same `value` as you do in this scope
}

ReactiveSwift calls out to multiple observers as a synchronous "did set" side effect. So if we allow any of the observer to modify the property, it invalidates these guarantees for all the remaining observers to be notified.

If we buffer the mutation with a queue-drain model (i.e. what makes reentrant signals work), then we will violate the developer expectation of how properties work, since what you set via the setter would no longer be visible through the getter immediately.

[andersio]

Recursive or reentrant signals will be enabled by ReactiveCocoa/ReactiveSwift#797.

[andersio]

For the purpose of unidirectional data flow, you can employ a queue-drain model built around Signal and custom logic, so as to avoid fighting with these MutableProperty guarantees.

An example implementation: https://github.com/ReactiveCocoa/Loop/blob/develop/Loop/Floodgate.swift

[mluisbrown]

OpenCombine.PassthroughSubject seems to be implemented in a similar manner

@maximkrouk TCA uses CurrentValueSubject, and OpenCombine's version releases the lock before sending the new value to observers: https://github.com/OpenCombine/OpenCombine/blob/master/Sources/OpenCombine/CurrentValueSubject.swift#L87

Thanks for the input @andersio. Simply by changing the sendLock in Signal from a Lock.make() to Lock.PthreadLock(recursive: true) I get the behaviour I was trying to create without any changes to MutableProperty:

let property = MutableProperty(0)

var values: [Int] = []
property.producer.startWithValues { value in
  values.append(value)
  if value == 42 {
	property.value = 0
  }
}

property.value = 1
property.value = 2
property.value = 42

expect(values) == [0, 1, 2, 42, 0]

It would seem that even with ReactiveCocoa/ReactiveSwift#797 the above would not work? I guess that means that Store.state should not be modelled as a MutableProperty?

For the purpose of unidirectional data flow, you can employ a queue-drain model built around Signal and custom logic, so as to avoid fighting with these MutableProperty guarantees.

Ok, so that's a yes 👍

to tackle some legitimate recursive delivery scenarios (e.g. around first responder management)

Yes, RAS-TCA has exactly the same issues 😄