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

Subscription overlapping and auto-resubscribe lead to temporary "broken" state #2440

Closed
tmeasday opened this issue Aug 26, 2014 · 7 comments
Closed
Labels
confirmed We want to fix or implement it Project:DDP

Comments

@tmeasday
Copy link
Contributor

_1 Upvote_ Suppose you have two subscriptions a and b that both add() to the same document. Then if you have an autorun like:

Deps.autorun(function() {
  if (/* something */) {
    Meteor.subscribe('a');
  } else {
    Meteor.subscribe('b');
  }
});

Then, when the guard changes, the following will happen:

  1. We'll subscribe to b.
  2. b goes ready. We are now subscribed to both a and b. Due to the oft discussed "arbitrary" subscription precedence (e.g. Deep merging of documents #998), temporarily a's document will win.
  3. a is un-subbed, and we get b's document.

It's surprising because we aren't explicitly subscribing to two subs that publish the same document. And it leads to broken behaviour because the sub goes ready with the "wrong" data.

It's also a pretty common scenario when you consider the same subscription getting called with different reactive arguments.

Repro: https://github.com/tmeasday/publication-auto-override

Perhaps b should not become ready until a has finished unsubbing?

@n1mmy n1mmy added the confirmed label Sep 5, 2014
@n1mmy
Copy link
Contributor

n1mmy commented Sep 5, 2014

Yeah, I see what you mean. This is a good point. Not totally sure what the right answer is here.

One answer (basically what you said) is: subscriptions that occur within the context of an autorun have their onReady callbacks held back until all subs that were pending for deletion have completely unsubbed. This is a little tricky to implement, but does make sense with the model.

@mitar
Copy link
Contributor

mitar commented Sep 7, 2014

Is this related to #1173? I think it maybe should be addressed together?

@tmeasday
Copy link
Contributor Author

tmeasday commented Sep 8, 2014

@mitar I think it's a bit different -- that's purely a problem with reactivity breaking, and the fix is pretty simple I'd guess. I think I actually spotted the problem some time ago but never remembered to bring it up. Sorry!

This is a more subtle issue with data appearing wrong for a moment.

@n1mmy let me know if you can think of any workarounds apart from "makes sure the different subscriptions publish different records", which can be difficult in practice.

@hpx7
Copy link

hpx7 commented Oct 16, 2014

+1

@timhaines
Copy link
Contributor

Related problem where auto-resubscribing and getting different values for a document that was subscribed to in both subscriptions results in the observer's changed callback getting invoked after the onReady callback: https://groups.google.com/forum/#!topic/meteor-core/j4Z8Cf3XV_M

If we make sure the updated document has a different id, it results in the observer's added callback getting invoked before the onReady as expected.

@mitar
Copy link
Contributor

mitar commented Jun 3, 2017

temporarily a's document will win.

Just to be clear. It is not that a's document win as a whole. Only for overlapping top-level fields (those present in both a and b) fields from a win. If fields from b are present, which are not in a, then those are available. I think it is pretty rare to have same top-level field from two publications which are having different value, isn't it?

Am I understanding this correctly?

What are use cases when one subscribes to a different subscription as a whole inside autorun?

@hwillson
Copy link
Contributor

As mentioned in #2440 (comment), the chances of this being a problem are pretty low. While there is still an issue here, developer demand to have this fixed has also been quite low. As part of the on-going old issue cleanup process, I'll close this for now. If anyone feels differently (and is interested in working on a patch for this), please let us know. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed We want to fix or implement it Project:DDP
Projects
None yet
Development

No branches or pull requests

7 participants