Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

bug in version 2.1.0pre Feb 24, 2012 #387

Merged
merged 2 commits into from

6 participants

@pilavdzic

I tried upgrading to latest 2.1.0pre from 2.0.0 in my application and on a page with a lot of complex functionality, nested templates, bindings, etc, in certain cases depending on the data I get error:

Uncaught TypeError: Cannot call method 'dispose' of undefined

2.0.0 works reliably as expected.

My specific case is very complicated to reproduce so I am hoping that it is still helpful just to let you know about this, in case you already have an idea what this might be?

Thanks!

@mbest
Owner

Would you be able to provide the stack trace for the exception? I know that Chrome provides this in the console. Just click the little triangle next to the error message.

@pilavdzic

I'll try to get you more info, but the exception is rethrown and the stack trace stops at the place it is rethrown from, which happens to be jquery's .ajax onsuccess handler.

Within that success handler, when I step through each line of code step by step, it happens right after I set the value of an observable that many things depend on.

I tried to step into each function and step through but there is too much code, and large arrays of items that are processed many times, so I can't easily tell which data item or which bit of code is causing this.

Is there any way I can tell which bindings, subscription or dependentobservables are being triggered by that observable value changing and/or which one of them is throwing the error? Thanks.

@mbest
Owner

Thanks for working on that. Can you try a couple more things. First, can you try running it with my fork of Knockout. It's based on 2.1.0pre, but includes some changes that might solve this problem. It's at https://github.com/mbest/knockout/downloads

I have a suggestion on how to get the stack trace. Wrap the observable update in a try/catch block that outputs the stack trace:

try {
    myObs(newVaue); 
} catch(e) {
    console.log(e.stack);
}
@pilavdzic

Good idea, thanks, here is the trace you requested:

TypeError: Cannot call method 'dispose' of undefined
    at evaluateImmediate (http://localhost:51485/_cassette/asset/Scripts/global/knockout-latest.js?a53e17c72b0cb3fe1b726f657b08042aa879597d:1172:66)
    at [object Object].evaluatePossiblyAsync [as callback] (http://localhost:51485/_cassette/asset/Scripts/global/knockout-latest.js?a53e17c72b0cb3fe1b726f657b08042aa879597d:1142:13)
    at http://localhost:51485/_cassette/asset/Scripts/global/knockout-latest.js?a53e17c72b0cb3fe1b726f657b08042aa879597d:860:34
    at Object.arrayForEach (http://localhost:51485/_cassette/asset/Scripts/global/knockout-latest.js?a53e17c72b0cb3fe1b726f657b08042aa879597d:83:17)
    at Function.notifySubscribers (http://localhost:51485/_cassette/asset/Scripts/global/knockout-latest.js?a53e17c72b0cb3fe1b726f657b08042aa879597d:856:22)
    at Function.valueHasMutated (http://localhost:51485/_cassette/asset/Scripts/global/knockout-latest.js?a53e17c72b0cb3fe1b726f657b08042aa879597d:933:79)
    at Object.observable [as selectedProjectIndex] (http://localhost:51485/_cassette/asset/Scripts/global/knockout-latest.js?a53e17c72b0cb3fe1b726f657b08042aa879597d:922:28)
    at Object.switchToProjectIndex (http://localhost:51485/_cassette/asset/Scripts/Project/Project.js?7be9e59da63443dd21d4bc34ff7e44764f714ac0:228:19)
    at Object.success (http://localhost:51485/_cassette/asset/Scripts/Project/Project.js?7be9e59da63443dd21d4bc34ff7e44764f714ac0:203:35)
    at http://localhost:51485/_cassette/asset/Scripts/global/jquery.min.js?9eb9ac595e9b5544e2dc79fff7cd2d0b4b5ef71f:2:14784

You are also correct, using your new code fixes the issue, so looks like you already took care of this, thanks!

But if there is there some internal ko variable or easy way to tell which bindings, subscription or dependentobservables are being triggered by an observable value changing I would appreciate it if you could tell me how to do this anyway. Normally the bugs are in my code, so a way to determine where errors come from more easily would be very helpful to me.

@pilavdzic

P.S. What's new since 2.0 in this version other than bugfixes? Can I use the $index variable in foreach's now? Thanks

@mbest
Owner

It seems that you have a computed observable that triggers an update to itself. This causes it to be updated recursively. Left unchecked, this would normally cause a stack overflow. In your case, you must be limiting the recursion in some way. My update that you tested adds a check to specifically prevent recursion, and it seems that 2.0.0 at least handles your case gracefully. Does this sound like what's happening in your case?

@pilavdzic

Not sure.

This is the only computed observable on that page:

viewModel.currentAward = ko.computed(function () {

    if (viewModel.awardIndex() == -1 || viewModel.selectedProject() == undefined) {
        return undefined;
    }
    else {
        return viewModel.selectedProject().Awards()[viewModel.awardIndex()];
    }
});

There are a lot of subscriptions and other things going on on that page so I can't be sure though.

Any ideas how I can find out which one?

@mbest
Owner

I've created a simple example that seems to reproduce the problem: http://jsfiddle.net/mbest/nEQbY/

@mbest
Owner

There are a lot of subscriptions and other things going on on that page so I can't be sure though.

I don't see any updates in the computed function. It could be something that's happening in a subscription though, related to #341

@SteveSanderson

The issue here is a subtle change introduced in commit bdf5c4d regarding nested evaluations and how subscriptions get disposed. The new mechanism, while more performant, can suffer from the _subscriptionsToDependencies array getting corrupted by a nested evaluation. This is about as close to a threading issue as you can get in JavaScript (without actually being multithreaded).

I'd say there are two main choices for how we handle this, depending on whether or not we regard nested evaluations to be a legitimate scenario or not.

  1. If it is legitimate for a ko.computed's evaluation process to call the evaluator function multiple times in a nested way, then we need to make the subscription/disposal logic able to robustly merge together all the subscriptions established down the call stack, and also negotiate disposal through the call stack too.

  2. If it's not legitimate for an evaluator to need inner evaluations of itself, then we can trivially just skip over inner re-evaluations. This is only a couple of lines of code. Then, the computed will just ignore writes to its dependencies until its own evaluation is completed.

I haven't yet thought of any even remotely sane situation where it's desirable for a ko.computed's evaluator to force itself to re-run recursively. So far, KO has left this scenario undefined. I'm now wondering if the best way to define it is just to say:

"A computed re-evaluates when any of its dependencies change. If dependencies change further still during evaluation, this does not restart evaluation. The next evaluation happens only after the previous evaluation has finished, in response to a subsequent dependency change."

With that definition, solution (2) makes most sense, and avoids various issues to do with infinite recursion. I appreciate this is rather obscure and awkward to make sense of, but does anyone have any thoughts on this?

@rniemeyer
Owner

I am in favor of option 2. I can't see a legit need for the evaluator to recursively call itself that is not better handled in other ways.

@mbest
Owner

Steve, I favor option 2 as well. I already implemented this in my "smart binding" fork and have attached the code changes here.

@pilavdzic

I don't need recursive evaluation either.

But in very complex relationships it would be very helpful to be able to see what all the dependencies are somehow. The order in which the subscriptions are triggered can be important and is difficult for me to determine. Is there any easy way to tell?

Thanks.

@RoyJacobs

Option 2 is my preferred scenario as well. If you have a complex set of dependencies, I don't think it's uncommon to have a situation where "A" references "B" while "B" references "A" (even if it's massively indirect). Having this automatically solved would be very nice.

I do have to second @pilavdzic by saying that a way to annotate subscriptions would be a 'nice to have', though that may be best left to a plugin that wraps the original ko.subscribable.

@mbest
Owner

I'd like to add that we should have some caution before making this change as it can break some existing code. Here's an example that uses recursion and works in 2.0.0: http://jsfiddle.net/nEQbY/4/

Here's the same example using my Knockout fork that prevents recursion: http://jsfiddle.net/nEQbY/5/

Notice that in the first case, the binding gets updated to 1, but in the second case it stays 3. Obviously code like this can and probably should be changed so that recursion isn't necessary. Maybe we should include a warning log whenever we've blocked recursion (at least in the debug build).

@SteveSanderson SteveSanderson merged commit e89e12a into master
@SteveSanderson

Excellent implementation - thanks very much, Michael!

I take your point about the possible change of behavior. This change, however, does make 2.1.0 more consistent with 2.0.0 in the real-world cases we know about (pilavdzic's original scenario) so on balance it is probably less breaking than not making the change at all. Let's review this if anyone discovers any other real-world cases where it makes a difference.

@mbest
Owner

Thanks, Steve. That sounds like a good plan.

@gilesbradshaw

That's interesting - I've changed all of my computeds to pure mainly because I assumed they always worked but now I get the "A 'pure' computed must not be called recursively" error - curious as to why we need an error when it's pure? and would it be sensible to make masking the error an option? I am using 3.2.0 alpha

@mbest
Owner

If you're getting that error, it must be because the computed isn't actually "pure", that is, your read function has side-effects.

@gilesbradshaw

Yes I see what you mean. My issue however was around the disposal of subscriptions see here #1390

This is crucial to my application because I extend observables to fetch and subscribe and unsubscribe to data on subscription and disposal. I am not sure if I am concerned about whether the computed are 'pure' or not. I will try and get my head round this and write more later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 15, 2012
  1. @mbest
Commits on Mar 20, 2012
This page is out of date. Refresh to see the latest.
View
11 spec/dependentObservableBehaviors.js
@@ -226,5 +226,16 @@ describe('Dependent Observable', {
value_of(timesEvaluated).should_be(0);
value_of(instance()).should_be(123);
value_of(timesEvaluated).should_be(1);
+ },
+
+ 'Should prevent recursive calling of read function': function() {
+ var observable = ko.observable(0),
+ computed = ko.dependentObservable(function() {
+ // this both reads and writes to the observable
+ // will result in errors like "Maximum call stack size exceeded" (chrome)
+ // or "Out of stack space" (IE) or "too much recursion" (Firefox) if recursion
+ // isn't prevented
+ observable(observable() + 1);
+ });
}
})
View
14 src/subscribables/dependentObservable.js
@@ -1,6 +1,7 @@
ko.dependentObservable = function (evaluatorFunctionOrOptions, evaluatorFunctionTarget, options) {
var _latestValue,
_hasBeenEvaluated = false,
+ _isBeingEvaluated = false,
readFunction = evaluatorFunctionOrOptions;
if (readFunction && typeof readFunction == "object") {
@@ -58,6 +59,14 @@ ko.dependentObservable = function (evaluatorFunctionOrOptions, evaluatorFunction
}
function evaluateImmediate() {
+ if (_isBeingEvaluated) {
+ // If the evaluation of a ko.computed causes side effects, it's possible that it will trigger its own re-evaluation.
+ // This is not desirable (it's hard for a developer to realise a chain of dependencies might cause this, and they almost
+ // certainly didn't intend infinite re-evaluations). So, for predictability, we simply prevent ko.computeds from causing
+ // their own re-evaluation. Further discussion at https://github.com/SteveSanderson/knockout/pull/387
+ return;
+ }
+
// Don't dispose on first evaluation, because the "disposeWhen" callback might
// e.g., dispose when the associated DOM element isn't in the doc, and it's not
// going to be in the doc until *after* the first evaluation
@@ -66,6 +75,7 @@ ko.dependentObservable = function (evaluatorFunctionOrOptions, evaluatorFunction
return;
}
+ _isBeingEvaluated = true;
try {
// Initially, we assume that none of the subscriptions are still being used (i.e., all are candidates for disposal).
// Then, during evaluation, we cross off any that are in fact still being used.
@@ -86,6 +96,7 @@ ko.dependentObservable = function (evaluatorFunctionOrOptions, evaluatorFunction
if (disposalCandidates[i])
_subscriptionsToDependencies.splice(i, 1)[0].dispose();
}
+ _hasBeenEvaluated = true;
dependentObservable["notifySubscribers"](_latestValue, "beforeChange");
_latestValue = newValue;
@@ -95,7 +106,8 @@ ko.dependentObservable = function (evaluatorFunctionOrOptions, evaluatorFunction
}
dependentObservable["notifySubscribers"](_latestValue);
- _hasBeenEvaluated = true;
+ _isBeingEvaluated = false;
+
}
function dependentObservable() {
Something went wrong with that request. Please try again.