Skip to content

Commit

Permalink
fix waitlist to ensure ready() is correct before next flush
Browse files Browse the repository at this point in the history 
Issue #742
  • Loading branch information
@cmather
cmather committed Aug 18, 2014
1 parent 89d237a commit 6922224
Showing 1 changed file with 16 additions and 9 deletions.
25 changes: 16 additions & 9 deletions lib/client/wait_list.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,15 +115,22 @@ WaitList.prototype.wait = function (fn) {

if (activeComp) {
activeComp.onInvalidate(function () {
// stop the computation
comp.stop();

// remove the computation from the list
self._comps.splice(_.indexOf(self._comps, comp), 1);

if (cachedResult === false) {
self._notReadyCount--;
}
// keep the old computation and notReadyCount the same for one
// flush cycle so that we don't end up in an intermediate state
// where list.ready() is not correct.

// keep the state the same until the flush cycle is complete
Deps.afterFlush(function () {
// stop the computation
comp.stop();

// remove the computation from the list
self._comps.splice(_.indexOf(self._comps, comp), 1);

if (cachedResult === false) {
self._notReadyCount--;
}
});
});
}
});
Expand Down

5 comments on commit 6922224

@tmeasday
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cmather you still need to trigger and invalidation if line 131 here makes self._notReadyCount go to zero. Otherwise there are situations where it'll be wrong.

I expanded the test case to fail again.

The TLDR:

  • you really can't rely on the inner autorun running again. It might not if we are no longer waiting!

The DR:

There's a legit use case where this is bad. It's not too complicated:

  1. You go to a route with a waitOn. Router.current().ready() is false for a bit, then true. The wait list is waiting on one handle.
  2. You re-route to somewhere else with no waitOn. Now, Router.current().ready() is briefly false. This can trigger weird and unexpected behaviour.

I haven't traced it through exactly in my use case but it seems pretty obvious that this problem could happen if you "change" the state of the waitlist (by decrementing _notReadyCount) without triggering a changed() event. It feels like a house of cards to rely on some other code running to "fix" the wrong-ness, as:

a) there's guaranteed to be a time period where it's wrong, and
b) it's more or less guaranteed to be edge cases where the problem is never "fixed".

@cmather
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is your recommended solution to check whether self._notReadyCount has changed and if so change the outer readyDep?

@tmeasday
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well if it changed and went to 0, yes.

@cmather
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I added that change and the test is passing. I'm preparing a 0.9.2 release to use newer versions of iron:layout and iron:dynamic-template relating to Blaze changes that are coming this week.

@tmeasday
Copy link
Contributor

@tmeasday tmeasday commented on 6922224 Aug 28, 2014 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.