Ember.computed task.isRunning == "You modified isRunning twice in a single render" [SOLVED]


#1

I’m encountering a “You’ve modified xxx twice in a single render” error when using ember-concurrency tasks inside computed properties.

Here’s an extremely minimal example of a component that demonstrates the problem:

double-render-bug.js:

import Ember from 'ember';
import { task, timeout } from 'ember-concurrency';

export default Ember.Component.extend({
	task: task(function * () {
		yield timeout(1000);
		return 'done';
	}),
	taskInstance: Ember.computed(function() {
		return this.get('task').perform();
	}),
	resultValue: Ember.computed.alias('taskInstance.value'),
	isRunning: Ember.computed.alias('task.isRunning'),
});

double-render-bug.hbs:

{{isRunning}} / {{resultValue}}

The main purpose behind the real version of this is to use computed properties to have my data automatically loaded (and then transformed and rendered) when parameters (for example ‘month’ or something) are changed. The methodology works beautifully, except when I try to use the ‘isRunning’ value to determine whether my data is currently loading or not.

It appears that what happens is:

  1. The getter for isRunning is fired, and ember-concurrency returns ‘false’, since no task instance has yet been created.
  2. Then the getter for resultValue is fired, which in turn causes the task to be performed. Internally, ember-concurrency then sets the ‘isRunning’ value to true, which in turn causes my Ember.computed.alias to be re-evaluated.
  3. Everything blows up, since the isRunning is being “modified twice”.

Is this a bug in ember? Is this a bug in ember-concurrency? Do I have any practical recourse, other than some complicated ‘debounce’ on the isRunning flag?

(NOTE - to see this work successfully, simply change the isRunning to a hard-coded value, like false.)

Thanks in advance!


#2

If you need the task to be started at component initialization, you should probably start it in its init() method.


#3

No, that’s kind of the whole point – I DON’T need it to be started at component initialization, I need it to happen whenever the relevant input parameters change. (I know, there aren’t any in this simple example, but there are in my real life case.) Often, these parameters AREN’T available at initialization, but become available later.

So in order to do it ‘for real’, I’d have to add an observer, and manually call the ‘perform’ on the task – everything that isn’t “the ember way”, and more complicated to boot. (Not to mention there’s nothing to say I wouldn’t have exactly the same problem there, as I’d be doing basically the same operation.)

Tasks couldn’t be more perfectly suited to exactly the usage I have above… Except for this niggling problem.

I’ve been looking at the ember-concurrency code, and the isRunning is simply computed from numRunning. I can see only a single spot (verified with conditional breakpoints) that seems to adjust this property. I did notice that the property is initialized to zero in a mixin – is there any chance that:

// Excerpted from -task-state-mixin.js
...
numRunning: 0,
...

in the mixin somehow counts as one ‘setting’ of the property? Hence the later:

// Excerpted from -scheduler.js
_startTaskInstance: function _startTaskInstance(taskInstance) {
    ...
    var task = taskInstance.task;
    task.decrementProperty('numQueued');
    task.incrementProperty('numRunning');
    ...

counts as a second modification of the same property?

I’m a bit puzzled as to where the second modification comes from…


#4

I don’t know if I’d call this a bug — but it’s certainly behaviors that looks like it should work, but because of how ember and ember-concurrency work together, it’s creating the double render.

In similar situations in the past, I’ve used the didReceiveAttrs component hook for firing off concurrency tasks when the inputs change. Have you tried that at all? (It doesn’t necessarily tell you which property changed — so, I could see some additional challenges if that is important).


#5

@Spencer_Price I was under the impression that you could do something like this:

  didUpdateAttrs: function(attrs){
    // if we want to only pay attention to certain attrs, we can do this:
    let new = attrs.newAttrs.<attr name>.value;
    let old = attrs.oldAttrs.<attr name>.value;
  },

so you could see what specifically changed.


#6

Yes, I think technically you can…but, I was under the impression that using attrs is not necessarily “public” API, so it may break in the future.

(Also, doesn’t it only have a .value if it’s a binding versus passing a primitive value? Does this.attrFor(attrs.newAttrs, 'name') still work? Haven’t tried in forever — feel like that may have been removed — because that was meant to account for the binding/primitive differences).


#7

Thanks for all the suggestions, but most of these seem like a lot of extra convolutions – and based on the code in ember-concurrency, I still can’t see where there is a ‘double-set’ of a property.

I’m going to see if I can produce a test case without ember-concurrency, to submit to the ember team. In the meantime, I’m attempting to add an observer on the ‘isRunning’ from the task, which then sets (but only once) another copy of ‘isRunning’ that I can reference (bleah).


#8

OK, here’s a pure ember version of the issue – no ember-concurrency required:

import Ember from 'ember';

export default Ember.Component.extend({
	task: Ember.Object.extend({
		isRunning: Ember.computed.gt('numRunning',0),
		numRunning: 0
	}).create(),
	taskInstance: Ember.computed(function() {
		this.get('task').incrementProperty('numRunning');
		return Ember.Object.create({value: 'done'});
	}),
	resultValue: Ember.computed.alias('taskInstance.value'),
	isRunning: Ember.computed.alias('task.isRunning'),
});

The issue appears to be the fact that one of the computed properties has an (unintentional) side-effect – incrementing a property that has already been utilized in computing another property.

Here’s the workaround that I was able to use:

  1. Remove the Ember.computed.alias on task.isRunning.
  2. Turn my isRunning into a plain old property, no computed stuff.
  3. Create an observer on task.isRunning.
  4. Inside that observer, set my isRunning property, but ‘debounced’ so that it happens only ONCE.
    • I did this by setting my isRunning property inside an Ember.run.next
    • I safeguarded it by cancelling any outstanding Ember.run.next each time I submitted a new one.

Kind of convoluted. It might be nice if Ember provided an “Ember.computed.debouncedAlias” or something to take care of such cases…


#9

Can you share the code for this solution? It seems like you’re still doing a lot to work around the desire to put the perform of a task in a computed property.

If the didReceiveAttrs hook doesn’t meet your use case, I would advocate for an observer than a computed property with side effects (in fact: it seems you’re using a CP as an observer…so, perhaps it’s better to be semantically correct anyway)


#10

Also — I just saw your other post which sounds related. If you can share more about what you’re trying to do (or, even better, the code itself!) I’d love to help by getting a second set of eyes on it. My optimistic love of ember-concurrency leads me to believe that there might be a way to simplify everything, but these separate bits are hard to understand the larger picture.


#11

OK, first some background:

Background

The basic concept is that in many places in any application, you have components that:

  • Accept one or more parameters – parameters which are changed by the user (i.e., not static)
  • Load some data – possibly one set of data, possibly multiple sets of data
  • Do some transformation on the data
  • Display (i.e., render) the result

(In short, a pretty standard/normal workflow.)

It’s not hard to do this programatically – for example:

  • An observer on all your parameters…
  • …which calls a ‘loadData’ method…
  • …with yields to handle async…
  • …then transforms the data…
  • …then sets the data into a property (or properties) which are rendered.

Of course, you inevitably find that the real life case requires a few twists, and then you have to deal with live updates to your data coming in via MQ, users aborting in midstream (though ember-concurrency helps a log), etc…

Ember, on the other hand, really encourages a ‘declarative’ approach to this type of operation – a cascading set of computed properties that define what you want instead of how to go about the operation of getting it.

Once you buy into this ember concept, you find things becoming a LOT simpler, and a million times more maintainable.

Ember even provides things like PromiseArray and PromiseObject to handle async operations inside computed properties (after all, the entire point of computed properties is that you don’t have to care about the order of operations or waiting on the results – if things load a bit later, your template simply updates with the new data and redraws what is needed).

Where it really becomes beautiful, though, is when you use tasks – tasks automatically take care of all of the cancellation/cleanup for you, and they expose awesome ‘derived state’ properties! This means that the complexity of using PromiseArray or PromiseObject completely goes away – you simply reference the task.value property, which is null or undefined initially, and gets filled in when the task is done!

So how am I trying to do this in real life?

Example

Let’s assume that I have a simple component that displays invoices for a given customer and month. That component then could look something like this:

export default Ember.Component.extend({
    month: null,       // These parameters will be set externally -- presumably
    customer: null,    // we'll have some sort of dropdown/picker.
    invoiceDataTask: task(function * (month,customer) {
        return yield this.get('store').query('invoice',...month & customer parameters...);
    }),
    invoiceDataTaskInstance: Ember.computed('month','customer',function() {
        const month = this.get('month');
        const customer = this.get('customer');
        return this.get('invoiceDataTask').perform(month,customer);
    }),
    invoiceData: Ember.computed.reads('invoiceDataTaskInstance.value'),
    isLoading: Ember.computed.reads('invoiceDataTask.isRunning')
});

and we have a functionally complete component js file! As you can see:

  • Any time the user changes the month or customer, the data will automatically reload (the task will be refired).
  • The data shows up in the invoiceData property as soon as the load is finished.
  • I can immediately tell whether or not my data has loaded, by simply checking isLoading

In short, a very simple, declarative way of expressing this.

My hbs file would then look something like:

<div class="{{if isLoading 'loading' 'loaded'}}">
	{{#each invoiceData as |invoice|}}
	    ...all sorts of lovely invoice display stuff...
	{{/each}}
</div>

Trouble in Paradise

Where the problem crops up is with the isLoading declaration – what happens is:

  • Ember chooses to compute this property first, and it returns ‘false’ since no task is (yet) running.
  • Ember then chooses to compute the invoiceDataTaskInstance, which fires off the task
    • This then triggers a recompute of isLoading, because ember-concurrency has incremented the ‘number of instances running’ variable.
  • Recomputing isLoading triggers the infamous ember ‘You modified “isLoading” twice in a single render.’ error, because the template makes use of this flag.

Solutions

I went through a number of different solutions, and my almost-last one was the observer thing that I tried – this worked, but was (as you say) a bit complicated.

However, in looking at the problem again, it seemed to me that the true crux of the issue was the fact that the invoiceDataTaskInstance property was the one firing off the task (and setting the isRunning, but the CHECK for isRunning goes only against invoiceDataTask (a subtle difference). This is why ember is computing the ‘isLoading’ property first – all of its dependencies are satisfied, and it doesn’t need invoiceDataTaskInstance.

Modifying my declaration from:

    isLoading: Ember.computed.reads('invoiceDataTask.isRunning')

to:

    isLoading: Ember.computed('invoiceDataTaskInstance','invoiceDataTask.isRunning',function() {
        this.get('invoiceDataTaskInstance'); // If you don't "get" a property that you declared in your deps, it won't really be a dep.
        return this.get('invoiceDataTask.isRunning');
    })

then solved the problem – Ember now renders things in correct dependency order.

I’m not 100% positive I won’t still (in some way) be able to create a case in which this occurs again, but initial testing is positive.

Thanks for all the comments and help!


#12

Got it. Glad that you’ve got a working solution!

(ember-concurrency is a thing of beauty, indeed).

For what it’s worth, for similar situations, I tend to do something like this (using didReceiveAttrs):

export default Ember.Component.extend({
  month: null,
  customer: null,

  invoiceData: Ember.computed.reads('invoiceDataTask.lastSuccessful.value'),
  isLoading: Ember.computed.reads('invoiceDataTask.isRunning'),
  
  didReceiveAttrs() {
    const { month, customer } = this.getProperties('month', 'customer');
    this.get('invoiceDataTask').perform(month, customer);
  },

  invoiceDataTask: task(function * (month, customer) {
      return yield this.get('store').query('invoice', { month, customer });
  })
});

#13

Thanks for the didReceiveAttrs option – it still feels much less ember-ish than just depending on properties in a computed :slight_smile:

The ember-concurrency guy posted a helpful link in the issue I opened there:

https://github.com/machty/ember-concurrency/issues/179

basically, it looks like the task instance already supports checking .isRunning – it just wasn’t in the documentation that I was looking at. Doing an Ember.computed.alias to the taskInstance.isRunning solves the problem completely.