New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Race condition when compose.view-model is bound to an object literal expression #240

Closed
gregdouglas opened this Issue Jul 16, 2016 · 8 comments

Comments

Projects
None yet
6 participants
@gregdouglas

gregdouglas commented Jul 16, 2016

Hi

I think I am experiencing some kind of race condition when using a nested compose element inside a repeat.for. I've extracted it to a really simple example which can be seen in this gist:

https://gist.run/?id=99128d8bf51c3a47a7ad4ffdc2ed42d8

The compose content is output twice in certain cells - sometimes it renders correctly which makes me think there is some kind of race conditions is going on here..

I am running latest Aurelia skeleton and posted on the gitter channel to ensure I wasn't the only one experiencing this.

A workaround seems to be to create a view model - and not use the html only.

Thanks in advance for helping me out.

@gregdouglas gregdouglas changed the title from BUG - Race condition when using compose in nested repeats to Possible race condition when using compose in nested repeats Jul 16, 2016

@CasiOo

This comment has been minimized.

Show comment
Hide comment
@CasiOo

CasiOo Jul 17, 2016

It seems the problem only occurs when the view-model is defined inline view-model.bind="{ data : x + i * 12 }". Bind it to an existing object, or link it to a VM file, and the problem disappear.

I found out that compose's viewModelChanged is being executed for the cells where the content is duplicated. This only happens for the duplicate content cells. I have not yet been able to figure out why it's happening. Not much help from the call stack, as it points to the task queue being drained.

Any guidance on where one might look to find the bug? I seem to be a little stuck tracking it down :).

CasiOo commented Jul 17, 2016

It seems the problem only occurs when the view-model is defined inline view-model.bind="{ data : x + i * 12 }". Bind it to an existing object, or link it to a VM file, and the problem disappear.

I found out that compose's viewModelChanged is being executed for the cells where the content is duplicated. This only happens for the duplicate content cells. I have not yet been able to figure out why it's happening. Not much help from the call stack, as it points to the task queue being drained.

Any guidance on where one might look to find the bug? I seem to be a little stuck tracking it down :).

@CasiOo

This comment has been minimized.

Show comment
Hide comment
@CasiOo

CasiOo Jul 17, 2016

So there seems to be two problems causing the bug to happen.

  1. Compose doesn't wait for the composition engine to complete composing before processing a new instruction.
    Compose should either cancel the previous composition, or wait for the previous composition before starting another one. composer.compositionEngine.compose returns a promise that could be chained or waited upon.

  2. The second problem is caused by the connect-queue. The connect-queue has a minimumImmediate variable for how many bindings should be processed before being queued. It turns out the duplicated cells have been queued, which in itself isn't a problem.
    The problem appears when the binding expression is reevaluated, meaning it takes the inline view-model expression and creates another VM. Since it always creates a new object instance for inline view-models, it thinks the binding has changed, and therefore fires the viewModelChanged.

CasiOo commented Jul 17, 2016

So there seems to be two problems causing the bug to happen.

  1. Compose doesn't wait for the composition engine to complete composing before processing a new instruction.
    Compose should either cancel the previous composition, or wait for the previous composition before starting another one. composer.compositionEngine.compose returns a promise that could be chained or waited upon.

  2. The second problem is caused by the connect-queue. The connect-queue has a minimumImmediate variable for how many bindings should be processed before being queued. It turns out the duplicated cells have been queued, which in itself isn't a problem.
    The problem appears when the binding expression is reevaluated, meaning it takes the inline view-model expression and creates another VM. Since it always creates a new object instance for inline view-models, it thinks the binding has changed, and therefore fires the viewModelChanged.

@EisenbergEffect

This comment has been minimized.

Show comment
Hide comment
@EisenbergEffect

EisenbergEffect Jul 17, 2016

Member

I suspected the connect queue might be involved. @jdanyow What do you make of this?

Member

EisenbergEffect commented Jul 17, 2016

I suspected the connect queue might be involved. @jdanyow What do you make of this?

@chrisjasp

This comment has been minimized.

Show comment
Hide comment
@chrisjasp

chrisjasp Dec 7, 2016

Any solution or work around to this issue yet?

chrisjasp commented Dec 7, 2016

Any solution or work around to this issue yet?

@EisenbergEffect

This comment has been minimized.

Show comment
Hide comment
@EisenbergEffect

EisenbergEffect Dec 13, 2016

Member

@jdanyow Any thoughts on this?

Member

EisenbergEffect commented Dec 13, 2016

@jdanyow Any thoughts on this?

@jdanyow

This comment has been minimized.

Show comment
Hide comment
@jdanyow

jdanyow Dec 14, 2016

Member

If you switch view-model.bind="{ data : x }" to view-model.one-time="{ data : x }" it will work like you expect.

The problem is every time the expression is evaluated a new view-model instance is created due to the object literal expression in the binding. Ultimately looks like a bug in the compose where it's not handling view-model prop changes when they happen quickly.

Member

jdanyow commented Dec 14, 2016

If you switch view-model.bind="{ data : x }" to view-model.one-time="{ data : x }" it will work like you expect.

The problem is every time the expression is evaluated a new view-model instance is created due to the object literal expression in the binding. Ultimately looks like a bug in the compose where it's not handling view-model prop changes when they happen quickly.

@jdanyow jdanyow changed the title from Possible race condition when using compose in nested repeats to Race condition when compose.view-model is bound to an object literal expression Feb 20, 2017

@jdanyow jdanyow removed the question label Feb 20, 2017

@YardGnomeNinja

This comment has been minimized.

Show comment
Hide comment
@YardGnomeNinja

YardGnomeNinja Jul 18, 2017

Has any work been done toward resolving this? This feature is really the linchpin to my project. I've apparently been lucky I didn't come across this issue earlier.

I'm using object literal expressions in my binding, however the difference in my situation is that I'm using object literals to specify which files to bind to, like so:

<div repeat.for="question of step.questions" class="question">
    <compose
        if.bind="question.display"
        show.bind="question.visible"
        view-model="resources/elements/controls/${question.controlType}/${question.controlType}"
        view="resources/elements/controls/${question.controlType}/${question.controlType}.html"
        model.bind="question">
    </compose>
</div>

The issue manifests, similarly as explained above, that when I move an item between two arrays that are being rendered via a repeat.for, the moved item is rendered twice. I have verified that the destination array contains only one instance of the item.

At the very least, is there a way to one-time bind to a file as a stopgap or would that potentially cause other issues?

YardGnomeNinja commented Jul 18, 2017

Has any work been done toward resolving this? This feature is really the linchpin to my project. I've apparently been lucky I didn't come across this issue earlier.

I'm using object literal expressions in my binding, however the difference in my situation is that I'm using object literals to specify which files to bind to, like so:

<div repeat.for="question of step.questions" class="question">
    <compose
        if.bind="question.display"
        show.bind="question.visible"
        view-model="resources/elements/controls/${question.controlType}/${question.controlType}"
        view="resources/elements/controls/${question.controlType}/${question.controlType}.html"
        model.bind="question">
    </compose>
</div>

The issue manifests, similarly as explained above, that when I move an item between two arrays that are being rendered via a repeat.for, the moved item is rendered twice. I have verified that the destination array contains only one instance of the item.

At the very least, is there a way to one-time bind to a file as a stopgap or would that potentially cause other issues?

@YardGnomeNinja

This comment has been minimized.

Show comment
Hide comment
@YardGnomeNinja

YardGnomeNinja Jul 20, 2017

In the meantime, I found a super hacky way to get around it by using a setTimeout for 2000 ms before I initiate the moving of items between arrays, 1000 ms wasn't enough in my case. It's not the ideal solution, but it works well enough for my purposes. I only mention it in the event someone else comes across this issue and this could help them.

YardGnomeNinja commented Jul 20, 2017

In the meantime, I found a super hacky way to get around it by using a setTimeout for 2000 ms before I initiate the moving of items between arrays, 1000 ms wasn't enough in my case. It's not the ideal solution, but it works well enough for my purposes. I only mention it in the event someone else comes across this issue and this could help them.

StrahilKazlachev added a commit to StrahilKazlachev/templating-resources that referenced this issue Sep 17, 2017

fix(compose): await composition/activation
before applying new changes close aurelia#299, close aurelia#240
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment