Node bindings should be applied only after all state nodes have been updated

[denis-anisimov]

There is an issue #1050 which is the main reason for this issue.

The fix for the issue is a custom way to struggle against the order for a node with TemplateOverridesMap feature and the node which it refer to (the node with OverrideElementData feature).

The fix works for it but it means that in each similar situation one has to remember about this kind of issues and be aware of the order of message processing.

Normally the message processing should care about the order and process some nodes before others.

Here is the example of messages :

{"node":9,"type":"put","key":"8","feat":18,"nodeValue":75},{"node":9,"type":"put","key":"expanded","feat":17,"value":true},{"node":9,"type":"put","key":"selected","feat":17,"value":true},{"node":75,"type":"attach"},{"node":75,"type":"put","key":"routerlink","feat":3,"value":""},{"node":75,"type":"put","key":"node","feat":19,"value":8}

Node 9 refers to node 75 which has been attached (because attach events are processed first) but it has no yet feature 19 (OverrideElementData feature).
So the Binder doesn't know how to handle it.

This situation may be generic enough and changes for node 9 should not be processed before changes for node 75.
So some ordering in message processing might be required.

[Legioth]

I just realized that we've all been looking at this problem from the wrong direction. The original problem is that actions are performed eagerly when some part of the state tree is being updated instead of deferring the actions until the entire tree has been updated. This is easily done either through using Reactive.runWhenDependenciesChange or Reactive.addFlushListener instead of performing actions immediately in a property change listener or splice listener.

[denis-anisimov]

See https://reviewable.io/reviews/vaadin/hummingbird/1052 how it could be fixed.
But it has a number of issues.
Sometimes node 75 might be still handled by the Binder before it has feature defined.

[denis-anisimov]

@Legioth Will it really solve an issue ?
Because regardless of how processing is done there is still some order.
Yes, actions will be differed but if node 9 is processed before node 75 (deferred) it's still an issue.

[Legioth]

Yes, actions will be differed but if node 9 is processed before node 75 (deferred) it's still an issue.

I don't think there are any cases where it would matter in which order individual changes are processed, as long as the processing is done only after all state tree changes have been applied.

[denis-anisimov]

Yes, you are right. The order should not matter.

Except one aspect.
At the moment TEMPLATE_OVERRIDES feature is processed in the binder like this:

MapProperty overrideProperty = modelNode
                .getMap(NodeFeatures.TEMPLATE_OVERRIDES)
                .getProperty(String.valueOf(templateNode.getId()));
        if (overrideProperty.hasValue()) {

So it assumes that there is always TEMPLATE_OVERRIDES feature for the node. In this specific case it's OK.
But normally it should check modelNode.hasFeature(TEMPLATE_OVERRIDES) before retrieve it from the node. And in that case the order will matter.
Since it's the same issue: feature doesn't exist until message is processed. And there is no such event as "feature appeared".

[denis-anisimov]

I would suggest send all features (+their types) as a part of "attach" message and initialize them immidiately BEFORE (as it's done with attach messages now) any other actions in addition to deferred processing .
In that way we can always rely on metainfo about the node in the Binder. Reactive will take care about value updates.

[Legioth]

At the moment TEMPLATE_OVERRIDES feature is processed in the binder like this:

Yes, it assumes that the node has been updated, but not that any specific change events have been run for that node.

Why not also make this binder logic run deferred? In that way everything could work in the same way without any special case processing. The only question then is whether we could do something to let anyone discover when they've forgot to do something deferredly.

[denis-anisimov]

Actually we already has this kind of code that I'm referring to :
ElementTemplateBindingStrategy.createServerProxy:

private JavaScriptObject createServerProxy(StateNode templateStateNode) {
        assert templateStateNode.hasFeature(NodeFeatures.TEMPLATE);
        JavaScriptObject proxy = JavaScriptObject.createObject();

        if (templateStateNode
                .hasFeature(NodeFeatures.TEMPLATE_EVENT_HANDLER_NAMES)) {
            NodeList list = templateStateNode
                    .getList(NodeFeatures.TEMPLATE_EVENT_HANDLER_NAMES);
            for (int i = 0; i < list.length(); i++) {
                attachServerProxyMethod(proxy, templateStateNode,
                        list.get(i).toString());
            }
        }
        return proxy;
    }

If for some reason node has no yet TEMPLATE_EVENT_HANDLER_NAMES then it will not be executed and will not be executed in the future.

But with my suggestion we should not assume anymore that some feature has "static" values ( that never cahnges). If we initialize the feature in the very beginning then its values will be set later on at any time. So any read should use "bind" method.

[denis-anisimov]

Why not also make this binder logic run deferred? In that way everything could work in the same way without any special case processing. The only question then is whether we could do something to let anyone discover when they've forgot to do something deferredly.

What do you mean ?
Rewrite this code

if (templateStateNode
                .hasFeature(NodeFeatures.TEMPLATE_EVENT_HANDLER_NAMES)) {

to be deferred ?

Does Reactive handle automatically this ?

[Legioth]

Adjustments to the order of applying changes from the server will not be enough when we also add support for changing the model from JavaScript in the browser. This means that we cannot rely on reordering, but must instead make sure all change listeners can deal with changes happening in any order.

MapPropertyAddEvent, MapPropertyChangeEvent and ListSpliceEvent are all fired eagerly while changes are applied to the state tree, which means that any code that is run directly from such a listener may see an incomplete version of the tree.

After the tree has been updated, we are always running Reactive.flush() which runs anything that has been enqueued explicitly using Reactive.addFlushListener or indirectly using Reactive.runWhenDependenciesChange. Anything run in this way will see a complete version of a tree.

We want to reduce the risk of accidentally doing any processing eagerly since basically all processing we're doing might directly or indirectly depend on other parts of the tree. We cannot directly change the eager events to be fired lazily since some inner workings of Reactive depend on getting events eagerly to be able to schedule flush tasks to be run in the right order.

What we can do is to create an overload of each of the three addXyzListener methods that takes an additional boolean eager argument, update Reactive to pass true when adding listeners and let the single-argument version that is used by all other parts of the framework pass false instead. When a listener is not added as eager the actual listener will simply be wrapped in a call to Reactive.addFlushListener before being added to the list of listeners.

[denis-anisimov]

Sorry, I'm a bit lost. You are describing already some solution.
But the solution for what ?

Solution for the initial issue or solution for some suggestions mentioned here ?

[Legioth]

Solution for the root cause, which should also fix all the specific issues.

[denis-anisimov]

OK.......
Then how does this help to solve the issue ? :

What we can do is to create an overload of each of the three addXyzListener methods that takes an additional boolean eager argument, update Reactive to pass true when adding listeners and let the single-argument version that is used by all other parts of the framework pass false instead.

And still, what to do with this code:

if (templateStateNode
                .hasFeature(NodeFeatures.TEMPLATE_EVENT_HANDLER_NAMES)) {

?
This code is inside Binder (ElementTemplateBindingStrategy) and is called only once. There is no any code that reacts on changes .
It's a part of binding a dom element to a stateNode which is called from the ElementTemplateBindingStrategy.bind method. And this method is called once.
Is there a way to defer the code above so that it will be executed once TEMPLATE_EVENT_HANDLER_NAMES feature appears ?

[Legioth]

Then how does this help to solve the issue?

By starting at the root cause: the entire chain of processing that eventually ends up calling hasFeature is run eagerly before the entire tree has been updated. The problem is that the processing is being run with TreeChangeProcessor.processChanges somewhere on the call stack when it should instead be run through Reactive.flush() from MessageHandler.processMessage.

The solution would be to make sure that entire chain of processing is run through Reactive.flush(). Only changing this one specific line of code would just cause a similar problem to appear in some other place instead.

My suggestion for making state node events fire lazily by default would significantly reduce the risk of causing this kind of hard-to-trace issues in the future, even when state node changes can also be triggered from other sources than a list of changes arriving from the server.

[Legioth]

Adding assert Reactive.isFlushing() to strategic locations of the node processing code (e.g. ElementTemplateBindingStrategy.bind) would also help discover any cases when we're trying to make updates based on a node tree that is currently being updated.

[denis-anisimov]

By starting at the root cause: the entire chain of processing that eventually ends up calling hasFeature is run eagerly before the entire tree has been updated. The problem is that the processing is being run with TreeChangeProcessor.processChanges somewhere on the call stack when it should instead be run through Reactive.flush() from MessageHandler.processMessage

Yes, agreed.

The solution would be to make sure that entire chain of processing is run through Reactive.flush(). Only changing this one specific line of code would just cause a similar problem to appear in some other place instead.

Yes, agreed.

My suggestion for making state node events fire lazily by default would significantly reduce the risk of causing this kind of hard-to-trace issues in the future, even when state node changes can also be triggered from other sources than a list of changes arriving from the server.

What we can do is to create an overload of each of the three addXyzListener methods that takes an additional boolean eager argument, update Reactive to pass true when adding listeners and let the single-argument version that is used by all other parts of the framework pass false instead. When a listener is not added as eager the actual listener will simply be wrapped in a call to Reactive.addFlushListener before being added to the list of listeners.

These I don't understand.

I believe you are talking about listeners in 3 implementations of ReactiveValue: NodeMap.addPropertyAddListener , MapProperty.addChangeListener and and NodeList.addSpliceListener.
At the moment in many cases those methods already contains a wrapper of real action inside Reactive.addFlushListener ( looks like only splice event listener , not others).
Reactive doesn't add listeners at all using those methods or any else method. Computation adds a listener but it's a ReactiveChangeListener.

And I still don't see how it helps to avoid getting false for the call

if (templateStateNode
                .hasFeature(NodeFeatures.TEMPLATE_EVENT_HANDLER_NAMES)) {

in ElementTemplateBindingStrategy.createServerProxy.
This code doesn't calculate anything in NodeList if there is no the feature. So there is no dependency which it can listen to run this logic once some change happens.
So if for some reason the feature is not yet in the node when the code is executed the first time then it won't be ever executed.

Anyway, since it's my last day before vacation I won't proceed with this today.

[Legioth]

Oh, you're right. We could probably just make all three addXyzListener methods invoke the listener deferredly and then go through all users to remove the explicit Reactive.addFlushListener wrappers.

ElementTemplateBindingStrategy.createServerProxy is called from arbitrary DOM event handlers. I don't see any reason why a DOM event would ever be delivered while the tree is being updated (since JS is single-threaded), so this shouldn't be a problem in practice. This does however mean that assert Reactive.isFlushing() would cause false negatives. We could maybe instead assert that we're not inside TreeChangeProcessor.processChanges.

[denis-anisimov]

ElementTemplateBindingStrategy.createServerProxy is called from arbitrary DOM event handlers. I don't see any reason why a DOM event would ever be delivered while the tree is being updated (since JS is single-threaded), so this shouldn't be a problem in practice. This does however mean that assert Reactive.isFlushing() would cause false negatives. We could maybe instead assert that we're not inside TreeChangeProcessor.processChanges.

Right. This is a bad example.

But what if current code

 MapProperty overrideProperty = modelNode
                .getMap(NodeFeatures.TEMPLATE_OVERRIDES)
                .getProperty(String.valueOf(templateNode.getId()));
        if (overrideProperty.hasValue()) {

is rewritten like this :

if ( modelNode.hasFeature(NodeFeatures.TEMPLATE_OVERRIDES)){
 MapProperty overrideProperty = modelNode
                .getMap(NodeFeatures.TEMPLATE_OVERRIDES)
                .getProperty(String.valueOf(templateNode.getId()));
        if (overrideProperty.hasValue()) {

.....
}

[Legioth]

I don't understand your point.

ElementTemplateBindingStrategy.bind should be run outside TreeChangeProcessor.processChanges to avoid seeing an inconsistent tree, anything else would be a bug that we should be able to detect quite easily with suitable asserts.

The logic that deals with override nodes should be written in a way that lets it detect whenever a new template override is added, since the first one might be added in any round trip. Wrapping the code that adds the listener in an hasFeature check would be broken regardless of how we deal with the order of tree updates within one round trip.

[denis-anisimov]

ElementTemplateBindingStrategy.bind should be run outside TreeChangeProcessor.processChanges to avoid seeing an inconsistent tree, anything else would be a bug that we should be able to detect quite easily with suitable asserts.

Yes.

The logic that deals with override nodes should be written in a way that lets it detect whenever a new template override is added, since the first one might be added in any round trip. Wrapping the code that adds the listener in an hasFeature check would be broken regardless of how we deal with the order of tree updates within one round trip.

Should be , yes.
But what if it's not written in that way? Will we able to detect this ?

[Legioth]

But what if it's not written in that way? Will we able to detect this ?

We will if there's a test for it, or if a demo somewhere uses it.

The conclusion is probably that we might want to think about how to prevent accidentally using hasFeature in the wrong way, since it has the potential of causing quite subtle bugs. But that is probably a separate separate discussion.