Fix MutableProperty deadlock

[olegshnitko]

No description provided.

[NachoSoto]

This was implemented this way on purpose: the source of truth should be the producer, not the signal.

The implementation is now worse, so I would call this a workaround, not a fix. I'd love to find a real fix for the underlying issue (SignalProducer.start), not just working around it in MutableProperty.

[filblue]

@NachoSoto, could you please elaborate on why the source of truth for the property should be the producer? By all means, PropertyType has hot semantics. All the truth about the PropertyType at any given moment is its always-present current value + value changed events (which is Signal).

PropertyType implementation does not require a buffer. It can be considered a special case of the buffer(1) with hot semantics, bringing all the unnecessary and complex locking and replaying logic of the buffer, which is not really needed. This PR makes PropertyType implementation simpler by completely removing all the notorious buffer problems.

[filblue]

@jspahrsummers, will you please share your opinion?

[NachoSoto]

PropertyType implementation does not require a buffer. It can be considered a special case of the buffer(1) with hot semantics, bringing all the unnecessary and complex locking and replaying logic of the buffer, which is not really needed. This PR makes PropertyType implementation simpler by completely removing all the notorious buffer problems.

For one reason: code reuse. PropertyType has the exact same semantics as SignalProducer.buffer, so there is no reason to duplicate this functionality. Instead of only fixing this one by reimplementing the logic, the best thing to do to keep the codebase maintainable is to fix the underlying issue instead of sweeping it under the rug.

[andersio]

A glimpse to the past - this was once implemented but reverted in #2622 (#2622 (diff)).

For this particular implementation, it slightly changed the semantic by breaking the guarantee of MutableProperty.producer always giving out the latest value, regardless of the liveness of its respective property. On the other hand, if you make another copy specifically for the producer with thread-safe guarantees, it would probably result in a SignalProducer.buffer replica as @NachoSoto suggested.

[filblue]

Thanks @NachoSoto and @andersio, your points are reasonable with respect to the state of RAC today.

Just for the sake of a broader discussion on the future of the framework, let me bring a question to the table. It may sound heretical, but:

Is the buffer really an essential and useful part of the framework?

To my understanding, its existence contradicts second of the three key RAC advantages over other Rx frameworks, declared in the README:

Where RAC differs from Rx, it is usually to:

• Create a simpler API
• Address common sources of confusion
• More closely match Cocoa conventions

And also there:

One of the most confusing aspects of Rx is that of “hot”, “cold”, and “warm” observables (event streams).
In short, given just a method or function declaration like this, in C#:
IObservable<string> Search(string query)
… it is impossible to tell whether subscribing to (observing) that IObservable will involve side effects. If it does involve side effects, it’s also impossible to tell whether each subscription has a side effect, or if only the first one does.
This example is contrived, but it demonstrates a real, pervasive problem that makes it extremely hard to understand Rx code (and pre-3.0 ReactiveCocoa code) at a glance.
ReactiveCocoa 3.0 has solved this problem by distinguishing side effects with the separate Signal and SignalProducer types. Although this means there’s another type to learn about, it improves code clarity and helps communicate intent much better.

I think that SignalProducer.buffer is exactly the hero of the story above. It brings a real, pervasive problem to the codebase due to its semantics ambiguity (leave alone the threading problem). SignalProducer created with buffer is not the SignalProducer that was meant to be 'side effects on subscription' brother of 'subscription side-effect-less' Signal. For clients, it's not clear anymore if:

1. there will be side effects on subscription
2. same side effects will be performed on the nth and n+1th subscriptions

What are the common use cases of the buffer?

• buffer(0) is not different from Signal.pipe()
• when a buffer(>0)'s caching/multicasting features are desired, PropertyType can be used instead.

PropertyType has the exact same semantics as SignalProducer.buffer

So why having two abstractions over the same implementation? What about deprecating the buffer and using PropertyType as a core building block, along with Signal and SignalProducer?

[andymatuschak]

I'd vote for killing buffer and focusing on Property, for the same reasons as I've argued elsewhere: encouraging signal producers to be less unbounded.

[filblue]

@neilpa, will you please share your opinion?

[mdiep]

PropertyType has the exact same semantics as SignalProducer.buffer

So why having two abstractions over the same implementation? What about deprecating the buffer and using PropertyType as a core building block, along with Signal and SignalProducer?

PropertyType has the same semantics as SignalProducer.buffer(1), not buffer generally.

But having said that, I can't think of a good use for buffer generally. (Except for testing SignalProducer; but that doesn't seem like a worthwhile cause.) Unless someone else has a good reason to keep it around, I'd support removing it.

[NachoSoto]

Unless someone else has a good reason to keep it around, I'd support removing it.

All the usages of SignalProducer.buffer in Khan Academy and Watch Chess (just 1 on each!), they can easily be replaced with Property.

The only limitation that would create is that Property can't emit errors, whereas that's something you could do with SignalProducer. With that said, I would be in favor of making this buffering semantic only available on Property as suggested.

[neilpa]

Agreed. I also think that buffer predates Property which is why this overlap exists.

[filblue]

Hey @NachoSoto, are there any reasons not to merge this soon?

[RuiAAPeres]

@filblue it seems @andersio PR suppresses this one.

[mdiep]

@olegshnitko Thank you for the pull request and for the discussion! #2764 has been merged, which supersedes this PR.