Disallow reentrant updates

[lorentey]

As an experiment, GlueKit's signal implementation currently provides support for reentrant sends; e.g., you are free to change the value of a variable from one of the subscribers to it:

let foo = Variable<Int>(4)
let c = foo.values.connect { v in 
    // If v is even, set it to a nearby odd value.
    if v & 1 == 0 { 
        foo.value = v + 1
    } 
}
print(foo.value)      // => 5
foo.value = 6
print(foo.value)      // => 7
c.disconnect()

Values that are sent while a signal is already sending values are queued up and sent asynchronously. Implementing this has been fun, but I wonder if it is worth the additional complexity.

Background

For reference, no reactive framework I know supports reentrant sends; they all deadlock on the first nested send invocation. However, KVO does support updating a property from one of its observers. When the property's value changes during one of KVO's signaling loops,

1. KVO performs a new, nested, synchronous signaling loop notifying all observers of the change immediately. The observer that was responsible for the nested change receives a nested observeValue(forKeyPath:,…) call.
2. Once the nested loop finishes, it completes the original signaling loop. However, it doesn't send the original notification payload, because that describes an outdated change — it updates the notification to match the latest value of the property.

KVO can do this because its signaling subsystem is written specifically for the purpose of sending change notifications, so it can update the notification payload as it is being sent, by merging interim changes into the original update.

GlueKit's Signal is a general signaling mechanism where in-flight values cannot be modified. So the only reasonable way to implement nested sends is to send their values asynchronously, after the currently running signaling loop completes. This means that the values that are received by observers are different in the two systems:

https://github.com/lorentey/GlueKit/blob/a0ed7f17fe0c05380270554c4ef79151f4cb473e/Tests/KVOSupportTests.swift#L217-L284

GlueKit's Signal is more consistent in the sense that everyone receives the same values, in the order they were sent. But KVO's implementation makes a little more sense in the context of change notifications — the value that observers see always matches what's in their notification payload.

Why is it a pain to support nested sends

The possibility that sending a value to an observer may trigger a nested send immensely complicates matters in a variety of common scenarios. For instance, sending a "welcome" value to new observers, like ObservableValueType.values does, becomes horribly complicated:

https://github.com/lorentey/GlueKit/blob/a0ed7f17fe0c05380270554c4ef79151f4cb473e/Sources/ObservableValue.swift#L62-L82

(The code is so complicated because it needs to handle the case where sink.receive(value) changes the value; for consistency, we want the sink to receive updates about its own changes, too. Also, connecting the sink to the source may send values to it by side effect (like values does), and those values need to be ordered consistently, too.)

I expect such nested sends will occur extremely rarely (if at all) in normal use-cases, but writing code to support them is unreasonably hard, and comes at a (probably) measurable cost of performance at runtime.

Furthermore, it is very easy to forget nested sends might occur; I'm sure the code already has instances where a nested send may lead to an inconsistent sequence of notifications.

How to remove support for reentrant sends

Signal can be easily simplified to remove support for nested sends; that's not a problem. Ideally a nested send would cause a trap, but deadlocking is fine, too, and is probably much easier to implement.

Other code that can be simplified can be found by a review of the codebase. (It's OK if we miss a couple.)

We need to consider whether KVO's support for reentrancy causes problems for the adapter code in KVO Support.swift. At first glance it doesn't, because recursive observeValue(forKeyPath:…) calls only happen for the actual observers that cause nested updates; other observers only get called strictly sequentially. (The fact that other reactive frameworks had no real issues with their own KVO adapters indicate this is correct.)

What about two-way bindings?

GlueKit includes rudimentary support for binding two updatables of the same Equatable type together.

https://github.com/lorentey/GlueKit/blob/a0ed7f17fe0c05380270554c4ef79151f4cb473e/Sources/UpdatableValue.swift#L151-L171

This feature has seen very limited use in production, and it is as yet unclear if the concept is robust enough. It has not even been updated yet for transactional changes. But supposing it is updated, removing support for nested sends does not cause problems at first glance — the equality check is there to prevent an infinite update loop, and it also does a good job of preventing even a single reentrant update. (Corner cases need to be dealt with, though. In the absence of nested updates, I don't think a Behavior's current value can ever differ from the one described in one its update Events, but I may be wrong.)

[lorentey]

A draft implementation is on the nonreentrant-signals branch. It improves Signal's send performance by about 25%.

However, the test failure below makes me a little worried. The test should trap, not just fail.

Bracketing sources like values become a lot simpler to implement without reentrancy, but they do still allow reentrant updates during the welcome/goodbye message, leading to bogus results. I'd expect the values source to catch the reentrancy and trap, not just ignore it—not reporting values from the reentrant change is not OK at all: