Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Question: multicast by default? #207

Closed
maxhoffmann opened this issue Mar 10, 2016 · 9 comments
Closed

Question: multicast by default? #207

maxhoffmann opened this issue Mar 10, 2016 · 9 comments

Comments

@maxhoffmann
Copy link

After working with most for a while (which I greatly enjoy, thanks for that) I still get bugs because I forgot to multicast a stream that has more than one listener. Most often I don’t spot this first and lose time debugging until I figure out that it’s a multicast issue again.

Would it be possible to make streams multicast by default and have unicast as an optimisation? As a user it’s much easier to have my graph work as expected and add unicast as an optimisation later if a stream has just one listener, than suddenly getting unexpected behaviour because a new listener was added.

@maxhoffmann
Copy link
Author

An example for a bug I just encountered because of this:

I have a stream that needs to fetch a value from the server on app startup. It receives an init event from initStream.take(1) and then creates a new request for this event inside of flatMap.

Because this value was used by more than just one stream later, the app suddenly made 4 requests to the server to get this static value, although I had take(1) on the initStream. It took some time to figure out that multicast was missing and the transformation was done for every listener.

@maxhoffmann
Copy link
Author

Another bug a I had because of missing multicast is with a window resize stream that had multiple subscribers. One of them didn’t get events when resizing too quickly, but with multicast it worked fine.

Compared to the bug above, these are two quite different behaviours although they have the same root cause. That’s why it always takes time to figure out if multicast is missing or if I did a mistake.

@briancavalier
Copy link
Member

tl;dr there are significant performance implications. For now, it's not likely to change, but we're actively investigating ways we could make it work.

It's been an open question, and there are good reasons on both sides. If you've seen any of the discussions about hot vs. cold streams in other libraries, this is related. It's a very tricky problem for push-based implementations (like all of the popular current generation reactive JS libs), and unfortunately (as you've seen), introduces additional cognitive load. Other approaches, such as pull-based (like the experiment here), can avoid the problems (tho they have their own issues to work out!).

One interesting discussion has been around the intent of reactive streams. Some feel that the intended use case is point-to-point, and doing something different from that requires action on the part of the developer. IOW, reactive streams aren't event emitters.

That's complicated by the fact that some streams, like most.just(x) or most.from(array), have quite different behavior than those created by @most/dom-event or most.fromEvent. The former are typically referred to as "cold", and reproduce their entire sequence for each observer, whereas the latter are referred to as "hot", and new observers simply "tune in" at some point (like turning on the radio)

The reason multicasting isn't the default in most.js is that doing the necessary bookkeeping at every node in the stream graph has significant performance implications. In the vast majority of "intermediate" nodes, multicasting isn't necessary. For example, take a function like:

const getTheStuff(url) =>
  most.fromPromise(getUrl(url))
    .filter(removeSillyThings)
    .map(extractOnlyTheGoodParts)

In that case, getTheStuff knows that multicast isn't necessary between fromPromise and map, nor between filter and map, because there can't possibly be multiple consumers on those intermediate nodes. It would be an unnecessary hit to force getTheStuff to pay an invisible cost by having most.js automatically multicast each.

There's been some work to reduce the cost in the case where a node has exactly one observer, but so far, it simply isn't possible to achieve the current non-multicasted level of performance.

So, right now, identifying the right points to allow multiple observers is the responsibility of the application developer. So, if you know that getTheStuff may have multiple consumers:

const getTheStuff(url) =>
  most.fromPromise(getUrl(url))
    .filter(removeSillyThings)
    .map(extractOnlyTheGoodParts)
    .multicast()

So, for now, we'll probably continue to experiment with ways to make multicasting "acceptably" fast. If that's possible, then it'll definitely be worth revisiting whether multicast-by-default is the right thing!

@maxhoffmann
Copy link
Author

Thanks for the long answer. That makes more sense to me now.

So in general is the rule to add multicast as soon as there are more than one subscriber or should I wait for things to break and add it then? I just had another issue where I’ve added multicast to a visibility stream which looks like this:

most.fromEvent('visibilitychange', document)
  .map(() => !document.hidden)
  .startWith(!document.hidden);

This resulted in streams not emitting events anymore that are subscribed to visibility. Removing multicast fixed the issue. Now I am a little confused, when I should add multicast and when I shouldn’t.

@briancavalier
Copy link
Member

Removing multicast fixed the issue

Interesting. I'll try to reproduce it and let you know what I find. If you discover more info, let me know!

@maxhoffmann
Copy link
Author

Looks like this is not supposed to break then? I’ll close this issue and open a new one for the visibility bug. Will try to find the root cause.

@briancavalier
Copy link
Member

Looks like this is not supposed to break then

Correct. I don't see why adding multicast would cause a problem.

Thanks for opening another issue!

@AlexGalays
Copy link
Contributor

AlexGalays commented Jun 5, 2016

I know this is closed and the library is designed like this on purpose, but just wanted to +1 this request as multicast by default seems to be consistently more natural in my usage of most.

in the short term, I think the API documentation of multicast could be clearer: Rather than being an opt-in optimization, you have to use multicast if the stream is subscribed from multiple points to get the more intuitive behavior we're used to:

i.e clicks.map(loadSomeAjaxData).switch() will not have the latest data if we don't include multicast somewhere after the map. This kind of very subtle bug is a pain.

Also, the doc could be more precise about where we need to use multicast. It's less obvious what should be done with setups like most.fromEvent(...).flatMap(x => most.fromEvent(...)).switch() (contrived example)
should the inner stream also be multicast-enabled for the whole resulting stream to be shareable ?

@AlexGalays
Copy link
Contributor

Just in case you don't receive notifications on edits; @briancavalier

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants