can.Map.attr() returns stale value for a "compute" type attr immediately after updating under certain conditions

[justinbmeyer]

@UselessPickles commented on Wed May 31 2017

Discovered in v2.3.27

If I have a can.Map with two "compute" properties:

    var DataModel = can.Map.extend({
        define: {
            triggersChangeToValue: {
                type: "compute"
            },
            value: {
                type: "compute"
            }
        }
    });

    var dataModel = new DataModel({
        triggersChangeToValue: false,
        value: 42
    });

AND I have created a compute from one of the fields in the map:
var valueComputePassedToAnotherControlFor2WayBinding = dataModel.compute("value");

AND I have bound a change listener to that compute:
valueComputePassedToAnotherControlFor2WayBinding.bind("change", function() {});

AND I change the value of that field in response to the other field changing, then immediately read the value of the changed attribute, I get the old (stale) value:

    dataModel.bind("triggersChangeToValue", function() {
        this.attr("value", 9000);
        console.log(this.attr("value")); // logs 42, but expected 9000!!!
    });

    // Trigger the change that should convert 'value' from 42 to 9000
    dataModel.attr("triggersChangeToValue", true);

This unexpected behavior seems to be triggered by a very particular combination of factors. For example, if I change EITHER property definition to not be "compute", then it works properly. Or, if I simply don't bind a change listener to my valueComputePassedToAnotherControlFor2WayBinding, then it also works as expected.

This is a complicated situation that is best demonstrated, rather than attempting to explain fully in words. Here is a jsfiddle demonstrating the problem, with several comments pointing out aspects of the situation that seem to trigger the unexpected behavior:

http://jsfiddle.net/n491v2xq/4/

[justinbmeyer]

@UselessPickles let me know if you want to pair on this; otherwise, it's not super high priority for us right now.

[UselessPickles]

@justinbmeyer I stepped through the code for a while, and started getting lost in the setter/getter logic for map attributes that are "computed".

When trying to read the attr immediately after setting it, I get into compute.get, where it ends up returning this.value, which is stale.:

        get: function () {
            var recordingObservation = can.__isRecordingObserves();
            if (recordingObservation && this._canObserve !== false) {
                can.__observe(this, 'change');
                if (!this.bound) {
                    can.Compute.temporarilyBind(this);
                }
            }
            if (this.bound) {
                if (recordingObservation && this.getDepth && this.getDepth() >= recordingObservation.getDepth()) {
                    ObservedInfo.updateUntil(this.getPrimaryDepth(), this.getDepth());
                }
                return this.value; // <--- this.value == 42 (old, stale value)!!!
            } else {
                return this._get();
            }
        },

If I don't have a change listener bound to valueComputePassedToAnotherControlFor2WayBinding, then in that same method, this.bound is false, and it returns this._get(), which returns the correct updated value.

I noticed that this._get() uses this.lastSetValue.get() to get the current value, which is correctly 9000 (the immediately recently set value).

Is the solution as simple as updating the above get() method to return this.lastSetValue.get() instead of this.value in the if(this.bound) block?

How confident are you on unit test coverage of computes? Should I just try making this change and running unit tests? Any other insight into what the correct solution may be? What is the purpose of this.value vs this.lastSetValue? Is it intentional that this.value is not updated until the batch completes?

[UselessPickles]

Clarification: the implementation of this._get for compute that uses this.lastSetValue.get() is specifically a special implementation of _get provided by _setupAsyncCompute. So the solution is not as simple as I suggested, because this.lastSetValue exists only for "async" computes. I could hackishly check for this.lastSetValue and use it if available, but that's getting ugly and even less clear as to whether it is correct.

Again, this is getting deep into unfamiliar territory for me. Why is the attr's compute setup as an async compute in this situation? what is the purpose? etc...

[UselessPickles]

Looking at the implementation of _setupAsyncCompute has me even more confused now. When does this.value ever get set for async computes?

            this._set = function (newVal) {
                if (newVal === self.lastSetValue.get()) {
                    return this.value;
                }
                return self.lastSetValue.set(newVal);
            };

[UselessPickles]

Maybe the correct solution is to call this._get() from inside get(), rather than directly accessing this.value? The default implementation of _get simply returns this.value, so the behavior is unchanged for "normal" computes, but for various special computes with overridden _get methods, it will correctly get the value. Are there situations where we specifically do NOT want to call _get() and trust that this.value is a correct/cached value?

        get: function () {
            var recordingObservation = can.__isRecordingObserves();
            if (recordingObservation && this._canObserve !== false) {
                can.__observe(this, 'change');
                if (!this.bound) {
                    can.Compute.temporarilyBind(this);
                }
            }
            if (this.bound) {
                if (recordingObservation && this.getDepth && this.getDepth() >= recordingObservation.getDepth()) {
                    ObservedInfo.updateUntil(this.getPrimaryDepth(), this.getDepth());
                }
            }
            return this._get(); // <--- always use this._get
        },

[UselessPickles]

I think I'm getting closer. I stepped through what happens when I set the value. I end up in the proto_compute code here:

        set: function (newVal) {
            var old = this.value; // old == 42
            var setVal = this._set(newVal, old); // setVal == 9000
            if (this._setUpdates) { // <-- this is true, but it's a LIE!!!
                return this.value; // this.value == 42
            }
            if (this.hasDependencies) {
                return this._get();
            }
            if (setVal === undefined) {
                this.value = this._get();
            } else {
                this.value = setVal;
            }
            updateOnChange(this, this.value, old);
            return this.value;
        },

Refer back to the _setupAsyncCompute method:

            this._setUpdates = true; // <-- LIES!!!
            this.lastSetValue = new can.Compute(initialValue);
            this._set = function (newVal) {
                if (newVal === self.lastSetValue.get()) {
                    return this.value;
                }
                return self.lastSetValue.set(newVal);
            };

So... what is the correct solution? Should the asyncCompute set this._setUpdates = false, or should it explicitly set this.value inside of it's custom _set implementation?

[UselessPickles]

I confirmed that can v3 does NOT have this same bug. I hope that someone can use what I have reported to compare v2 code to v3 code and determine whether the v2 code can easily be updated to get rid of this bug.

[UselessPickles]

After some more experimentation, looking at code, etc., I have come to the conclusion that I am completely lost. The code for v3 still has the _setUpdates flag that on the surface appears to be a lie, but v3 does not have this bug, so it must not mean what I think it means.

Can someone that was involved with the design/development of compute code, etc., look at this?

[justinbmeyer]

@UselessPickles I can help though I'm pretty busy this week giving a training. Is there a time Monday we could pair on it?

[rjgotten]

FYI; There's a good chance the root cause for your bug is not to be found within async computes and/or computed can-map-define properties.

I ran into this exact same problem with stale values coming from a nested compute in some of my own code recently and it does not match the above conditions.

Mine was a case of the get in a compute that uses the { get, set, on, off } syntax being indirectly dependent on an updated value of a regular, i.e. , not-computed, property on a can.Map that was bound to in the compute's on( update ) to run the passed update function.

Indirectly dependent, because one would pass through several layers of computed get define properties on other maps to reach the stale value. Note though that those inbetween layers all had their getter correctly run and were, infact, not returning stale values. Only at the very last value; the regular not-computed property, which was actually what the top-level compute's update was bound to, would the stale value appear.

I wish I could produce a reduced test-case of my own occurence for you, but the code in question was a massive piece of logic related to implementing a faceted search and the moment I start decoupling, the bug resolves itself. As it stands, all I can do is inform you and hope that it helps assist in tracking down a cause.

I sincerely hope this isn't another case or side-effect of nested computes being evaluated in wrong order. Those issues seemed very nasty. Regardless; good hunting!