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

Tracker flush cycle should yield occasionally if not invoked via Tracker.flush #3901

Closed
pdavidow opened this Issue Mar 10, 2015 · 11 comments

Comments

Projects
None yet
2 participants
@pdavidow

pdavidow commented Mar 10, 2015

https://github.com/pdavidow/meteor-issue-infiniteLoop

Login by creating new user. Hit Import button to get infinite loop.
The culprit has something to do with method "isPieceListDisabled" in template.js.

Note that this problem was very difficult to narrow down, as system simply hangs.

(I whittled away plenty of code to reproduce, and probably could whittle away some more.)

@glasser

This comment has been minimized.

Member

glasser commented Mar 10, 2015

Hmm, there does look like a lot of code there still that could be whittled down.

@pdavidow

This comment has been minimized.

pdavidow commented Mar 10, 2015

Ok, please have a look now.

@glasser

This comment has been minimized.

Member

glasser commented Mar 10, 2015

Hi. Just to make sure I understand properly: you're saying that when you make your app be smaller (fewer than 3 subscriptions, 4 methods, 2 transforms, the use of accounts-ui and accounts-password, etc), the bug goes away? So this is some sort of bug that relies precisely on the combination of accounts-password, accounts-ui, transforms, and having three separate subscriptions?

@pdavidow

This comment has been minimized.

pdavidow commented Mar 10, 2015

No, I am not saying that at all.
But, if you insist, I'll try to condense even more.

@glasser

This comment has been minimized.

Member

glasser commented Mar 10, 2015

Yes, please see https://github.com/meteor/meteor/wiki/Contributing-to-Meteor#reporting-a-bug-in-meteor for advice on how to usefully show a bug.

@glasser glasser closed this Mar 10, 2015

@pdavidow

This comment has been minimized.

pdavidow commented Mar 10, 2015

Please have a look now.

@glasser

This comment has been minimized.

Member

glasser commented Mar 11, 2015

Thanks, that's a lot more clear.

Every time isDisabled is evaluated, it's within a Tracker computation, which watches for reads from reactive data sources and reruns the computation when those sources change.

In each run, you create a brand new Uhoh. You then call isStarted on it, which looks up the "isStarted" key in the ReactiveDict. Tracker notices this and decides that it will rerun when the isStarted key in the ReactiveDict changes. You then immediately go and change the "isStarted" key, which leads Tracker to decide that the computation needs to be rerun at the next flush cycle.

So after rendering the page, Tracker re-runs the computation. But since you have a brand new Uhoh this time through, everything said above happens again. And again and again.

If you remove the line that sets isStarted to false (and eg, just default to returning false without updating the reactiveDict), everything works fine.

So, the fact that it keeps recomputing isn't a bug. It's working as intended.

On the other hand, it would be nice if the behavior here wasn't so catastrophic: if this didn't turn into an infinite loop in Tracker.flush.

There are three ways I can think of to avoid this.

(1) Tracker.flush should eventually die with an error if too many computations get executed. You could of course run into false positives here if you have a long but finite chain of recomputations.

(2) The other would be to fundamentally change the API of Tracker.flush so that instead of returning when flush is done, it returns at some arbitrary point, and if you want to be notified when the flush is done, use Tracker.afterFlush. Then, after having run some large number of computations, it can yield to the event loop for a bit. However, this is a backwards-incompatible change!

(3) What I'd really suggest is the following. Tracker.flush() should still synchronously run the queue to completion. And this can hit an infinite loop. But if we're flushing just because somebody invalidated a computation or ran afterFlush (with no explicit call to Tracker.flush()) it should be OK to yield to the event loop occasionally.

@glasser glasser reopened this Mar 11, 2015

@glasser glasser changed the title from infinite loop to Tracker flush cycle should yield occasionally if not invoked via Tracker.flush Mar 11, 2015

@glasser glasser added the feature label Mar 11, 2015

@pdavidow

This comment has been minimized.

pdavidow commented Mar 11, 2015

In the meantime before this feature gets implemented, it would seem that a good rule of thumb is never to use lazy-initialization with a reactive source (e.g., ReactiveDict, Session, ReactiveVar).

@glasser

This comment has been minimized.

Member

glasser commented Mar 11, 2015

Actually, it's also an infinite loop in Computation.recompute.

@pdavidow You can use lazy initialization on a reactive source as long as that source itself isn't being recreated over and over!

glasser added a commit that referenced this issue Mar 11, 2015

Avoid infinite loops in the Tracker flush cycle
Tracker.flush() will still guarantee that all computations and
afterFlush callbacks have been fully flushed, but the implicit flush
cycle started by an invalidation or afterFlush call will yield to the
event loop for 10ms every thousand computations.

Fixes #3901.

glasser added a commit that referenced this issue Mar 11, 2015

Avoid infinite loops in the Tracker flush cycle
Tracker.flush() will still guarantee that all computations and
afterFlush callbacks have been fully flushed, but the implicit flush
cycle started by an invalidation or afterFlush call will yield to the
event loop for 10ms every thousand computations.

Fixes #3901.

glasser added a commit that referenced this issue Mar 12, 2015

Avoid infinite loops in the Tracker flush cycle
Tracker.flush() will still guarantee that all computations and
afterFlush callbacks have been fully flushed, but the implicit flush
cycle started by an invalidation or afterFlush call will yield to the
event loop for 10ms every thousand computations.

Fixes #3901.

@glasser glasser closed this in 14887d8 Mar 12, 2015

@pdavidow

This comment has been minimized.

pdavidow commented Mar 27, 2015

I just tried this in METEOR@1.0.5, and the problem is still there: the system hangs.

I also disagree with the classification (in https://rbcommons.com/s/meteor/r/25/) of this coding style as a 'bug'. Granted that in the watered down example I presented it is silly to code that way, but in deeply nested code with reusable components I see this as a very valid scenario.

@glasser

This comment has been minimized.

Member

glasser commented Mar 28, 2015

The change is not in 1.0.5, which was a small cherry-pick release. It will be in 1.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment