onBeforeAction runs endlessly

[manuel-schoebel]

Hi,

i have a onBeforeAction that uses this.subscribe(...) and also reactive Session vars and also calls pause() at some time. It all works well on 0.8.2 with IR 0.7.1.

Also I log a simple this.ready().

At some point, I guess when the reactivity kicks in, the onBeforeAction runs endlessly and the log says this.ready() = true.

Is this a known issue? Something to do about it? Reproduction needed (I think I could do it over the weekend, maybe)?

[manuel-schoebel]

I guess this relates to #741

[cmather]

It's possible there's an infinite invalidation loop. It's been reported in a few places. There will definitely be a workaround to prevent it. But then the deeper question is can we prevent users from getting into this situation in the first place somehow. But need to see some code so I can reason about it.

[manuel-schoebel]

I guess this reproduces the problem. Basically it is the client side join in an onBeforeAction.

https://github.com/DerMambo/ir_onBeforeBug

[manuel-schoebel]

One workaround seems to work if you use the waitOn function for the "main" subscription. Then subscribe to all depending subscriptions in a onBeforeAction. But this does not work if you want to use subscription in global onBeforeActions or have more than one subscription you might want to "join".

[tmeasday]

Thanks very much for the reproduction @DerMambo. Very helpful.

The issue is that you are waiting on a new subscription at the same time as calling this.ready(). This is hitting a bug in the wait list.

An easy work around is to not wait on the child subscriptions.

@cmather I've added a failing test to IR/wait_list that highlights the problem. Haven't really thought too hard about a solution but I thought I'd wait for your thoughts.

[tmeasday]

Happy to work it out on my own if you don't have time to think about it -- assign it back to me if so.

[cmather]

Ok cool thanks guys. Will take a look on the next cycle shortly

On Aug 7, 2014, at 8:42 PM, Tom Coleman [email protected] wrote:

Thanks very much for the reproduction @DerMambo. Very helpful.

The issue is that you are waiting on a new subscription at the same time as calling this.ready(). This is hitting a bug in the wait list.

An easy work around is to not wait on the child subscriptions.

@cmather I've added a failing test to IR/wait_list that highlights the problem. Haven't really thought too hard about a solution but I thought I'd wait for your thoughts.

—
Reply to this email directly or view it on GitHub.

[cmather]

@tmeasday, Can you summarize the issue? Is there an infinite loop created here somehow?

[tmeasday]

Yeah. You can see in https://github.com/EventedMind/iron-router/blob/devel/test/client/wait_list.js#L58-L70 that the autorun runs endlessly (we bail out after too many iterations to avoid infinite loops).

@DerMambo's reproduction caused an infinite loop directly in IR.

[cmather]

Got it. Looking at this today.

[cmather]

It looks like there's a few issues here:

1. @DerMambo is looking for a way to sequentially wait on subscriptions. For example given s1 and s2 where s2 has a parameter that requires data from s1 he'd like to wait to subscribe to s2 until s1 is complete. Additionally, the entire ready state of the controller should be false until both of these subscriptions are ready. We don't have a good pattern for this currently.
2. If you happen to call ready() and then subsequently add an item to the list by calling wait you can end up in an infinite invalidation loop. It's not 100% clear to me that the test case that Tom created and @DerMambo's issues are the same, but it seems like they are.

From a WaitList design perspective we need to figure out what the rule is here. Given some code like this:

var waitlist = new WaitList;
var handle = new Blaze.ReactiveVar(false);

Deps.autorun(function () {
  waitlist.ready();
  waitlist.wait(function () { return handle.get(); });
});

What should the expected behavior be? The current behavior is an infinite invalidation loop. The reason for this can be explained with these steps:

1. We call waitlist.ready() which returns true because the list is empty.
2. We call waistlist.wait which changes the ready state to false and invalidates the outer comp
3. The WaitList onInvalidate callback removes the computation from the waitlist list so that it doesn't get duplicated when the computation reruns.
4. Next tick and flush begins
5. Computation reruns, repeating this process

One potential solution is to not allow ready() and wait() to be called from the same computation. But this seems sort of limiting. I'm thinking of other solutions so thoughts are welcome...

[cmather]

One potential solution is to only deprecate the _notReadyCount in an afterFlush callback.

https://github.com/EventedMind/iron-router/blob/devel/lib/client/wait_list.js#L67

So the code would look like:

if (cachedResult === false) {
  Deps.afterFlush(function () { self._notReadyCount--; });
}

This would allow the count to stay the way it was for one flush cycle, preventing the infinite loop. But not sure how to prove correctness here. @tmeasday, thoughts?

[tmeasday]

@cmather I took another look at this particular issue and I realise that in fact and infinite loop is the expected behaviour, as we are essentially altering readiness inside a ready check.

I think the failing test cases should still be fixed however.

[cmather]

Expected, but probably fixable :-) doing a dep change in an after flush could fix it. And I guess since I'm using a counter the ready() value should always be up to date. Looking forward to checking out your solution.

On Aug 14, 2014, at 12:48 AM, Tom Coleman [email protected] wrote:

@cmather I took another look at this particular issue and I realise that in fact and infinite loop is the expected behaviour, as we are essentially altering readiness inside a ready check.

I think the failing test cases should still be fixed however.

—
Reply to this email directly or view it on GitHub.

[tmeasday]

Is it fixable? The code can be boiled down to:

var list = new WaitList;
Deps.autorun(function() {
  if (list.ready()) {
    list.wait(function() { return false; });
  }
});

Wouldn't the expected behaviour here be:

1. The list is ready, add the failing guard.
2. The list is now not ready, remove it.
3. Go to 1.

Seems like a classic infinite loop, ala setting + reading the same session var in the same autorun. I know that it's probably easy to make this kind of mistake in subtle ways (and maybe we can detect it and log something helpful), but fundamentally, how can we fix it without breaking the waitlist's behaviour?

[cmather]

Repeating my comment from the other commit:

I think the right approach is to detect this condition and throw an error. I have an implementation locally that I will try to push either tonight or when we get back from our family trip. The gist is: When a user calls wait() make sure the current computation or any of its ancestors is not already tracked as a dependent. If so, throw an error with suggested fixes which include:

1. Only check ready() after all wait() calls have been made or
2. Move the ready() call into its own sub computation so that it's isolated

[tmeasday]

Sounds reasonable. Still think we have to change the dep in the onInvalidate or there will be edge cases where this.ready() is wrong in autoruns.

[cmather]

@tmeasday, can you explain what you mean by this.ready() could be "wrong" in autoruns? I think I know what you mean (explained below) but I'm getting confused by the "change the dep in the onInvalidate" solution so I might not be understanding the problem.

One problem I see here is that in the onInvalidate callback I decrement _notReadyCount and pull out the inner computation from the list. It's possible that the active comp is invalidated and any code that calls ready() between the invalidation and the next flush will see true (i.e. notReadyCount is 0) and then once the flush completes, the ready state goes back to false.

Given the above, the rule that ready() should always return the correct value does not hold. So I'll need to think of a solution.

[cmather]

In 0.9.1 this condition should throw an error and ask you to isolate your ready() call or make sure that all wait() calls are done before ready() within the same computation.