Infinite Loop Problem

[colincoder]

I recently switched to your persistence node and ran into an issue that could be fixed with a simple? change. Here's the problem. I am using the package to remember the state of a UI Graph node. This graph node outputs its state whenever a new value is given it. This output feeds to a set-shared-state node. The input to the graph is feed from a matching get-shared-state node to reset the graph on a restart. But as the get-shared-state node also outputs whenever a matching set-shared-state receives data, an infinite loop results. I have implements a work around to only allow the get-shared-state output to get to the graph on init, and block all others.
A more graceful solution would be to enhance the get-shared-state node to be configurable to trigger on init, or trigger on a new value being stored, or both.
Do you have any interest in doing this? If not I am inclined to clone the node and attempt it myself.
Regards,
Colin

[lorenwest]

Hi @colincoder,

This module has built-in loop detection here, so the question is - how does your installation differ in a way that the loop detection fails?

The resolution isn't to add configuration complexity - it's to understand why loop detection isn't working, and make it more robust.

Could you put a few debug statements around this loop detection code to see why it thinks the value has changed, causing subsequent publishes?

[colincoder]

Hi,
Thanks for the quick reply. Here's what I tried. It's a bit long-winded but I wanted to be complete. But the executive summary is that when comparing the 2 items with
if (typedNewState === node.value) {return}
are they are objects and equal (as they are in my case), the comparison fails because === for objects only tests that they refer to the same location in memory. Obviously not ever the case here.

Here's the details. First some version info:
2 Jan 23:34:58 - [info] Node-RED version: v1.3.4
2 Jan 23:34:58 - [info] Node.js version: v12.22.1
2 Jan 23:34:58 - [info] Linux 5.10.17+ arm LE
2 Jan 23:35:02 - [info] Loading palette nodes
2 Jan 23:35:15 - [info] Dashboard version 2.29.0 started at /ui

And here's a test flow:

[{"id":"54d46577.ac7afc","type":"tab","label":"Flow 1","disabled":false,"info":""},{"id":"316cbedc.67fe72","type":"get-shared-state","z":"54d46577.ac7afc","state":"f273281c.5f06d8","name":"test","triggerOnInit":true,"x":230,"y":140,"wires":[["c131ca8c.b256e8"]]},{"id":"b664f3a8.01d0c","type":"set-shared-state","z":"54d46577.ac7afc","state":"f273281c.5f06d8","name":"test","triggerOnInit":true,"provideOutput":false,"outputs":0,"x":590,"y":140,"wires":[]},{"id":"c131ca8c.b256e8","type":"ui_chart","z":"54d46577.ac7afc","name":"test","group":"17ac92f6.2fc9f5","order":5,"width":0,"height":0,"label":"chart","chartType":"line","legend":"false","xformat":"HH:mm:ss","interpolate":"linear","nodata":"","dot":false,"ymin":"","ymax":"","removeOlder":1,"removeOlderPoints":"","removeOlderUnit":"3600","cutout":0,"useOneColor":false,"useUTC":false,"colors":["#1f77b4","#aec7e8","#ff7f0e","#2ca02c","#98df8a","#d62728","#ff9896","#9467bd","#c5b0d5"],"outputs":1,"useDifferentColor":false,"x":420,"y":140,"wires":[["b664f3a8.01d0c"]]},{"id":"7bb70071.aab6f","type":"random","z":"54d46577.ac7afc","name":"","low":1,"high":"100","inte":"true","property":"payload","x":350,"y":260,"wires":[["c131ca8c.b256e8"]]},{"id":"eaa6087c.e01b08","type":"inject","z":"54d46577.ac7afc","name":"","props":[{"p":"payload"},{"p":"topic","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","payload":"","payloadType":"date","x":150,"y":260,"wires":[["7bb70071.aab6f"]]},{"id":"f273281c.5f06d8","type":"shared-state","name":"test","lbl":"","tags":"","historyCount":"2","dataType":"obj","boolType":"bool","boolStrTrue":"","boolStrFalse":"","precision":"","numMin":"","numMax":"","unit":""},{"id":"17ac92f6.2fc9f5","type":"ui_group","name":"Refrigerator ","tab":"98bd6824.f06be8","order":1,"disp":true,"width":"7","collapse":false},{"id":"98bd6824.f06be8","type":"ui_tab","name":"Refrigerator","icon":"fa-thermometer-quarter","order":1,"disabled":false,"hidden":false}]

Here's the debug code at the point in state.js you indicated:

if (typedNewState === node.value) {return;}
      console.log("typedNewState: ");
      console.log(JSON.stringify(typedNewState,null,4));
      console.log("node.value: ");
      console.log(JSON.stringify(node.value,null,4));
node.prev = node.value;

and the result in the log (just the first pair as it ran away and I had to kill node-red).

typedNewState:
[
{
"series": [
""
],
"data": [
[
{
"x": 1641182822195,
"y": 69
}
]
]
}
]
node.value:
[
{
"series": [
""
],
"data": [
[
{
"x": 1641182822195,
"y": 69
}
]
]
}
]

As you can see, the contents of the 2 objects are identical, but the equality test in the line above didn't think so. My guess is testing 2 objects for the same content could be a bit daunting.

The Chart UI seemed to need to input as an object, hence my use of Object for the Data Type.

One possible solution is the stringify the objects when the node has a Data Type of Object and compare the resulting strings. To test this I changed your highlighted line to:

if (typeof node.value === 'object') {
  if (JSON.stringify(typedNewState) == JSON.stringify(node.value)) {return;}
else
  if (typedNewState === node.value) {return;}
}

The infinite loop did not occur.

I'm sure you could determine the Data Type from the node configuration, and I'm making the possibly rash assumption that the two objects will always stringify to the same string. In this case they are derived from the same source so should be, but that may not always be the case.

I hope this helps.
Colin

[colincoder]

Please ignore my liberal interchange of "to" and "the" above. If a sentence above makes no sense trying making a substitution! ;^)

[lorenwest]

Bravo! You're right - if they're objects, they won't be strictly equal. The above code looks great - if you want to submit it as a pull request I'll merge and re-publish.

Thank you for your help improving this library.

[colincoder]

Thanks again for the quick reply. I will do as you say. However I want to raise 2 points before that.

1. In the line if (typeof node.value === 'object') { don't you think it would be better to use the Data Type configuration of the node to determine if the type is Object? If you do, please point me to where to get that information from.
2. I still think that the ability to prevent triggering from a state change is worth considering. Here's why. If you look at the test flow I sent you this is what happens:

i) Some event occurs which injects a new value into the chart (that event could even be from a get-state init)
ii) The chart emits its state to the set-state node
iii) The set-state node sees this as a change of state and causes the get-state node to emit this new information to the chart
iv) The chart sees a new input and emits its state to set-state, even though the chart has not actually changed
v) set-state sees this as no change in state and (with my edit) does not trigger an infinite loop

In my chart flow example this is benign behavior, although obviously unnecessary as the goal is just to initialize the chart with historical information on restart, and nothing else. However, there may be a use where the number of times the state msg goes around the loop could be important (for example, if the number of messages was being counted for some reason).

Although I agree with your earlier comment about unneeded complexity, I note that the competing nodes from node-red-contrib-persist, which I was using, only emits on init, so without this mod (#2) yours is not truly a direct drop-in replacement. (It is much better than that contrib though, which is why I'm changing to it.) I realize that your software is trying to be more than just a persistence mechanism, which would only ever need to emit on init.

If you are in agreement with #2 I will complete the mod and send you a screenshot. Otherwise I will scrap it and just do #1. I understand that the #2 mod would need to read any existing deployed node configuration from before the mod, and set up for the same behavior in the new version.

Let me know on #1 and #2 and I will complete the work.

Regards,
Colin
PS. Hopefully no to/the transpositions in the above!

[lorenwest]

Hi Colin,

On issue 1 (above) I feel the most robust implementation is to always perform comparisons against JSON stringified values (regardless of data type) as this is the way it's stored on disk, and JSON.stringify() works for all persistable state.

if (JSON.stringify(typedNewState) == JSON.stringify(node.value)) {return}

On issue 2, as a state representation node I do not agree that state changes should be withheld from propagation. Non-changes (as in your example) are not propagated based on issue 1.

Further, I feel the most precise solution is for the client of this library to not invoke a state change unless a change actually occurred. Realizing that precision and complexity are often at odds, I feel that verifying the state changed before triggering a change event is a good alternative to requiring clients to be precise.

[lorenwest]

This library is designed to have multiple listeners to a state change event, and I understand the client making a change may not need (or want) to be notified of their own change.

If you happen to know you'll be the only client making changes and are only interested in the first init, then you could stop responding to the get-state after the first fire (on init).

If there could be many set-state clients, and you want to ignore get-state notifications resulting from your own set-state, then you could perform the same type of JSON.stringify() logic to prevent heavyweight operations (like re-rendering).

[lorenwest]

Apologies for the multiple posts... sometimes it takes a while for something to sink in :)

Of course, doing what I suggested in my previous post would require custom logic (in a JS node). I now see that if you only care about initializations, and want to not be notified on subsequent changes there isn't a way to do it without writing code.

It took me a while to fully understand the need, and if you'd like to propose an addition to the get-state node to only send updates on the first init, I'd be happy to review/merge it.

[colincoder]

Hi Loren,
Yes I completely agree with you. If the get-state is in a different flow or flows that do no issue the related set-state, then you'd absolutely need them to emit on state change. But if a get-state is in the same wired flow as the associated set-state then there is no need or benefit to emitting on a state change, in fact it might be detrimental to do so.
So here's my plan.
I will stringify all comparisons regardless of type, to deal with issue #1.
I am already well on the way to #2 and have implemented it as an additional checkbox as in this clip

Failing to check either results in a validation failure.
But I'd rather do it like this so bad data cannot be generated:

However, I cannot see how radio buttons are initialized from default or config settings (I'm new to this area of Node-red, although fully conversant with HTML, JS and jQuery.) Can you give an help or an example? The complexity is in initializing the radio buttons, extracting the selected value on Save (although that seems automatic for checkboxes), and using the original deployed state to correctly set them in an existing project, I have seen no other node use radios in their config (that I could crib from!) except the rpi-gpio in node, and that code is pretty complex.

It could also be done with a 3 entry dropdown, but with the same issues.

The checkboxes are the easiest, have none of these issues, and I already have them working.

[ BTW When I code and there are 3 possible states for some setting ( init, state change or both in this case) I like to cover all the bases and give the user all 3 options, as I can never be sure what the user's situation will be. ]

Give me your thoughts on the approaches to issue 2 above and I'll complete them. [ Check boxes are already done! ;^) ]

Regards,
Colin

[lorenwest]

Hi Colin,

Yes, radio buttons are the clearest and I don't recall ever using them in a Node-RED .html form. Node-RED does a bunch of work to get values in/out of the form, and I'll bet there are examples if you google enough (oops, I just revealed my coding technique).

You may have to do some work in jQuery in a trigger like I had to do here

[colincoder]

So I'm not the only one that uses Google as my teacher!
I will take a look at your suggestion but have been bogged down with a problem related to detecting changes when using Objects. I think I understand the cause, and the fix, but don't understand the mechanism that causes the problem.
Executive Summary: On start-up, the first change in state is detected and filed, but all subsequent ones are not seen as different and ignored, even when they are different.
Diagnosis: At the end of the update code the node.value is correct, but on the next call to update it has become the same value as the incoming msg so is not seen as different. My attempts to find out where it changed have failed so far.
Likely Cause: There are the following 2 lines in the update function in state.js:

node.prev = node.value;
node.value = typedNewState;

With Objects, the problem with the first line is that is really a pointer exchange when Objects, so afterwards node,prev has the same storage address as node,value. Then the next line changes it again to that of typedNewState, although that is a let variable so I don't know what that means. Clearly that is not right, and only affects Objects. So I changed the first part of the function as follows:

  async update(newState, fromMsg) {
    let node = this;
    let typedNewState = node.getTypedValue(newState, fromMsg);
    let newvalstr = JSON.stringify(typedNewState);
    let currentvalstr = JSON.stringify(node.value);
    if (newvalstr == currentvalstr) {return;}
	// With objects this makes prev and value share the same memory which 
	// makes the .prev value and .value value point to the same Object. 
	// So create new objects so that the comparison above detects an actual change.
	// This will work for non-objects too.
	//    node.prev = node.value;
	//    node.value = typedNewState;
    node.prev = JSON.parse(currentvalstr);
    node.value = JSON.parse(newvalstr);

That seemed to fix the problem, but I hate implementing a fix without understand why it does so! I'm not familiar with the async before the function definition but assume it means that code execution returns to the caller immediately rather than at the end of the function. It looks like a race-condition of some sort, and it's worth noting that the Chart node outputs its state several time in a row for a new input; no idea why.
I plan to continue my investigation, but any insight would be very helpful.
Regards,
Colin

[colincoder]

Further .... I've taken this as far as I can go. I have tested the Object fix quite extensively and it seems to be fine. I have also implemented the more flexible firing strategy using Radio buttons as we discussed. That also seems to work fine.
I can commit my changes to my fork at any time and issue a pull request (at your Git I assume). Not super familiar with the pull request process but assume it is straightforward.

[colincoder]

While awaiting your reply (no rush!) I realized that setState would benefit from the same ability to select the trigger cause, so I have added that too. The dialog now looks like this, with the radio buttons being hidden when Provide output is not checked.

This has a very nice side effect that, when initializing my Chart UI, I can just use a setState node with Provide output checked and Fire on Initializing, as above. No need for a getState node in this situation. Output of each to input of the other. The graph gets restored on start up but receives nothing when new data arrives and a new state is store, as it shouldn't. Very clean. Here's the test flow:

[
    {
        "id": "8a6e9c101f887f66",
        "type": "tab",
        "label": "Flow 1",
        "disabled": false,
        "info": "",
        "env": []
    },
    {
        "id": "fe9c640bc1ed2f3a",
        "type": "ui_chart",
        "z": "8a6e9c101f887f66",
        "name": "test",
        "group": "1aa32fa830b02b0e",
        "order": 0,
        "width": 0,
        "height": 0,
        "label": "chart",
        "chartType": "line",
        "legend": "true",
        "xformat": "HH:mm:ss",
        "interpolate": "linear",
        "nodata": "",
        "dot": true,
        "ymin": "",
        "ymax": "",
        "removeOlder": 1,
        "removeOlderPoints": "",
        "removeOlderUnit": "3600",
        "cutout": 0,
        "useOneColor": false,
        "useUTC": false,
        "colors": [
            "#1f77b4",
            "#aec7e8",
            "#ff7f0e",
            "#2ca02c",
            "#98df8a",
            "#d62728",
            "#ff9896",
            "#9467bd",
            "#c5b0d5"
        ],
        "outputs": 1,
        "useDifferentColor": false,
        "className": "",
        "x": 650,
        "y": 80,
        "wires": [
            [
                "eb0a161852f1cdce"
            ]
        ]
    },
    {
        "id": "eb0a161852f1cdce",
        "type": "set-shared-state",
        "z": "8a6e9c101f887f66",
        "state": "41871afc56162afa",
        "name": "test",
        "triggerOnInit": true,
        "triggerOnChange": false,
        "provideOutput": true,
        "outputs": 1,
        "x": 650,
        "y": 160,
        "wires": [
            [
                "fe9c640bc1ed2f3a"
            ]
        ]
    },
    {
        "id": "29c1a15bef033707",
        "type": "inject",
        "z": "8a6e9c101f887f66",
        "name": "",
        "props": [
            {
                "p": "payload"
            }
        ],
        "repeat": "60",
        "crontab": "",
        "once": true,
        "onceDelay": 0.1,
        "topic": "",
        "payload": "1",
        "payloadType": "num",
        "x": 150,
        "y": 80,
        "wires": [
            [
                "032275fb9e1bc20c"
            ]
        ]
    },
    {
        "id": "032275fb9e1bc20c",
        "type": "random",
        "z": "8a6e9c101f887f66",
        "name": "",
        "low": 1,
        "high": "100",
        "inte": "true",
        "property": "payload",
        "x": 300,
        "y": 80,
        "wires": [
            [
                "b416f0a7fae4bed9"
            ]
        ]
    },
    {
        "id": "b416f0a7fae4bed9",
        "type": "change",
        "z": "8a6e9c101f887f66",
        "name": "",
        "rules": [
            {
                "t": "set",
                "p": "topic",
                "pt": "msg",
                "to": "Per Cent",
                "tot": "str"
            }
        ],
        "action": "",
        "property": "",
        "from": "",
        "to": "",
        "reg": false,
        "x": 470,
        "y": 80,
        "wires": [
            [
                "fe9c640bc1ed2f3a"
            ]
        ]
    },
    {
        "id": "1aa32fa830b02b0e",
        "type": "ui_group",
        "name": "Default",
        "tab": "f6d97c5afe53d91c",
        "order": 1,
        "disp": true,
        "width": "6",
        "collapse": false,
        "className": ""
    },
    {
        "id": "41871afc56162afa",
        "type": "shared-state",
        "name": "test",
        "lbl": "",
        "tags": "",
        "historyCount": "2",
        "dataType": "obj",
        "boolType": "bool",
        "boolStrTrue": "",
        "boolStrFalse": "",
        "precision": "",
        "numMin": "",
        "numMax": "",
        "unit": ""
    },
    {
        "id": "f6d97c5afe53d91c",
        "type": "ui_tab",
        "name": "Home",
        "icon": "dashboard",
        "disabled": false,
        "hidden": false
    }
]

[lorenwest]

Been a bit busy. I agree with adding to get and set. Issue a pull request, and we can continue discussions on the code.

[colincoder]

My apologies for the delay in replying. As you know I have been trying to understand a strange and intermittent bug when storing Objects, that stopped state changes from being detected, even when they happened. After much hair-pulling I found that I could turn the problem on and off by changing a line in getState.js (around line 91) from:
payload:node.sharedState.value,
to
payload:JSON.parse(JSON.stringify(node.sharedState.value)),
It would seem that when the update(onInit) function is called (and processing is not suppressed by the config), it results in 2 Objects ending up pointing to the same memory, when they shouldn't. I can't say I understand the mechanism of the end result, but the problem seemed to go away. Maybe it will make more sense to you. But the end result is that when a change of state is noted by node.on("input' .... the node's node.value already knows about it so the code sees no difference. I do not understand how that happens.
Anyway, I will commit my changes in the next 24hrs (it's Sat 00:29GMT right now) and issue the pull request. Then you can study my changes and see if they make sense to you. Then we can discuss further as needed.