Client engine is not protected from several property change events when only one property is modified

[denis-anisimov]

See detailed description of the issue here #2460 (comment).

Technically I don't see a problem from the client side point of view when modification of only one property fires several events (that other properties are also changed).
One property may really lead to changes in other properties (somehow connected e.g. computed properties or for any other reasons).
If it happens and we update the property whose change event is fired (as a result of another property change) in the same request then the current client side property value wins: we will have old value and not the sent from the server. See the details why it happens int he link above.

So we should somehow protect the client engine from such situations.

• One way would be to completely change the design of client engine (its "asynchronous" nature). But I don't know whether it's feasible at all.
• Another way may be could be: change the MapProperty::setValue method signature so that it accepts one more argument with boolean type. true should be provided when this method is called from the message processing of server RPC requests. false if it's called from the client side. true is stored in the field inside MapProperty and any call with false is discarded if this field has value true. In the end of RPC message processing the value for all updated MapProperty instances should be set to false (though I don't know whether the property updates still happens when message processing is in progress). In this way all client side changes will be just ignored if the server side updates are not yet applied.

[denis-anisimov]

So here is the ugly draft which needs to be checked for all possible issue for the my second option from above:

TreeChangeProcessor::processPutChange

JsonValue jsonValue = change.get(JsonConstants.CHANGE_PUT_VALUE);
            Object value = ClientJsonCodec.decodeWithoutTypeInfo(jsonValue);
            property.setValue(value);

change to

JsonValue jsonValue = change.get(JsonConstants.CHANGE_PUT_VALUE);
            Object value = ClientJsonCodec.decodeWithoutTypeInfo(jsonValue);
            property.setValue1(value);

MapProperty change to :

public class MapProperty implements ReactiveValue {
    private final String name;
    private final NodeMap map;

    private boolean isRPC;

    public void setValue1(Object value) {
        isRPC = true;
        setValue(value);
    }

It's method syncToServer update to :

if (!Objects.equals(newValue, currentValue)) {
            if (isRPC) {
                return;
            }
           .........
 }
else {
    isRPC = false;
}

At least with this code the original issue with text-field becomes fixed.

[pleku]

This issue can cause a lot of confusion and lost time since it is very difficult to trace these client overrides in apps. As Denis mentioned, this can even be a feature instead of a bug for the web component to work this way, thus we should be able to handle the case properly.

I have no opinion on the implementation level. Does @Legioth have any opinions ?

[Legioth]

Is this the problematic order of events?

1. The client-side element has invalid: true and value: "".
2. Server response contains two updates for the same element: invalid: false and value: "foo". These values are updated to the state tree immediately, but not immediately propagated to the actual element.
3. invalid: false is applied to the element, which causes an event to be fired also for the value property.
4. The event handler sees that element has value: "" but the state tree has value: "foo". As a reaction to this, the state tree is updated to have value: "" and an RPC is scheduled.
5. When applying the value change to the element, the the current value from the state tree (i.e. "") is used. This is the same value that the element, and does thus not cause any additional events to be fired.

Could the solution in that case simply be that the scheduled task that propagates new values to the element captures the value that was received from the server instead of reading the current value from the state tree?

[denis-anisimov]

Could the solution in that case simply be that the scheduled task that propagates new values to the element captures the value that was received from the server instead of reading the current value from the state tree?

Yes and no/how ?

• Yes. That's what would be nice to do.
• How ?
  We expect that the value which is received from the server is written into the StateNode.
  And that's kind of the reason why do we have the StateTree/StateNodes at all.
  StateTree/StateNode is exactly the storage which we use to put the values received from the server (synced with the server).
  But this scenario breaks that suggestion.

So the question is how/where we should store server received values?
Create one more storage which will serve to this purpose ?
Looks like as an overhead.

Technically my suggestion with the meta-implementation (see above) is a variant of this: we put a value into MapProperty when request is received from the server side and ignore everything that comes from the client until this value is not applied to the client element.
So it's kind of the same approach implemented on the top of existing design.

[Legioth]

Do we have to explicitly store the value, or would it work to temporarily capture the value from the server in the event instance or directly through some closure?

[denis-anisimov]

Sorry, I don't think I understand you.

In reality we already have a bug in our code which is the cause of the #2460 (without any additional events from the client side).
I'm going to implement a way that I suggested and see what happens with the tests.
It requires less efforts than anything else at least and fixes this issue and #2460.

The issue with #2460 is: when we get the event about property change we have no information which property is changed. So the current code just goes through ALL the properties and check whether they are changed. The value property value has been received from the server along with invalid property value so the property in the StateNode is not the same as element's property value and value property is considered as updated.

Normally we should identify which property is the target of the event and send changes to the server ONLY for this property.
But we have no information in the event listener which property has to be updated when event is received.
EVENT types list and properties list are totally disconnected to each other and there is no way to get which ONLY ONE property should be updated when event is received.
Though it may be possible to have several different properties that are expected to be updated via the same event type.
As a result all properties are synced to the server ( if they are changed) and it's a problem for any property which has been changed in the same request as another one.

[denis-anisimov]

See the implementation here: #3331