Fix a race condition when a system is started on a different queue from its event serialising queue.

[andersio]

Resolve #37.

Why?

Due to the nature of prefix lazily starting the producer being prefixed, we cannot rely on on(value:) to ignite the feedbacks with the initial state. At the time prefix(value:) calls on(value:) for the initial value, the events-reducer producer has not yet been started yet. Consequentially, it would lead to dropped events when the system is instantiated on a queue different from the queue used for serializing events.

Explanation

The current operator application order is prefix(value:) followed by on(value:).

Recall that prefix(value:) is essentially a concat b. Since b would not start before a completes, the events -> reducer part is not started until a has finished sending out the prefix value.

When prefix(value:) sends out the initial value, on(value:) sends the initial value to the state signal, which in turn updates all feedback signals. But since the events-reducer producer hasn’t started yet (which is the flatMap(.concat) semantic), all events generated by feedbacks could potentially be delivered to the void, when the queue instantiating the system runs behind the queue serialising all feedback events.

How to fix it?

Having said that, prefix(value:) is guaranteed to have started the prefixed producer as part of the synchronous producer starting process. So we can address the issue by applying on(started:) after prefix(value:) to ignite the system, while having on(value:) applied instead before prefix(value:) to still keep the reducer-to-feedbacks path open.

[RuiAAPeres]

Due to the nature of prefix lazily starting the producer being prefixed, we cannot rely on on(value:) to ignite the feedbacks with the initial state. At the time prefix(value:) calls on(value:) for the initial value, the events-reducer producer has not yet been started yet. Consequentially, it would lead to dropped events when the system is instantiated on a queue different from the queue used for serializing events.

I really would like to understand this fix, since we extensively use ReactiveFeedback in our codebase, but I can't even understand the first paragraph. 😢

Do you think you could create a small diagram (like a timeline), so I can have a mental image of the events?

[andersio]

The race condition

Scenario A

Scenario B

The fix

[RuiAAPeres]

(beautiful diagrams, thanks! ❤️ )

(I am assuming S0 and S1, are related to State0 and State1?)

I am looking at scenario B, does it mean that the prefix S0 (or initial state) is never handled by the reducer?

[andersio]

@RuiAAPeres The reducer is constructed with the scan(initial:_:) operator, and has been fed with the initial state directly.

The reducer wouldn’t process any state directly, only events produced by feedbacks in response to the latest state. Then by processing an event, it computes a new state that gets fed back into the feedbacks.

[sergdort]

Are you saying that prefix operator starts source producer before you even subscribe to prefix?

[andersio]

No. LHS is started only after RHS (a producer emitting one value) completes, and that's the core of the issue.

[andersio]

RHS (the prefixed initial) sends out a value, feedbacks receive such value and produce an event, and the said event may be dequeued by the feedback loop queue before LHS (the reducer) starts on the initialising queue.

[RuiAAPeres]

Pardon my ignorance, but why do we need the aid of semaphore for this test?

[andersio]

Reproducing scenarios that require specific timing order often require manual synchronisation and sometimes repeated runs to lower chance of false positives, because the OS scheduler need not do you a favour when scheduling threads, and we can only create a controlled environment at best effort with these primitives.

Alternatively, this can be done by TestScheduler, but only if advance() supports draining tasks one-by-one.

[RuiAAPeres]

@andersio can you provide a case, in our codebase, where this bug manifests itself? https://github.com/Babylonpartners/babylon-ios/pull/6847

[inamiy]

Just to not forget to fix Property+System.swift... #38 (comment)

Also, we probably need to state <~ SignalProducer.system(...).skip(first: 2) instead since initial will be emitted twice.

[andersio]

@Inamy That doesn’t sound right to me. It shouldn’t emit the initial value twice afterwards.

[inamiy]

@andersio #38 (comment)
My bad. It doesn't seem required, so I will approve now 👍