FunctionNode: send() throws. Simplification possible?

[ralphwetzel]

There are two quite similar implementations of send() in nodered.js:

1) send(msg) @ class Node: https://github.com/phoddie/node-red-mcu/blob/ec7bce69735e996d788d53795b8cce68232a53ae/nodered.js#L241-L270

2) send(msd, _msgid) @ class FunctionNode ... which quite heavily walks through msg only to set _msgid and then relays to send(msg) @ class Node: https://github.com/phoddie/node-red-mcu/blob/ec7bce69735e996d788d53795b8cce68232a53ae/nodered.js#L563-L580

Issue: There's a bug @ line 573 that triggers Exception: FunctionNode.prototype.send: cannot coerce null to object! in case output == null. The same guard as in line 258 (if (output) ...) is necessary to fix this.

But: Do we really need this twice? send(msg) @ class Node is almost identical to the second implementation; both handle _msgid slightly different. I haven't checked the standard Node RED sources if this difference is demanded. If it is, I would propose to give send(msg) @ class Node a second parameter _msgid & handle the whole stuff @ class Node. This should simplify code management & additionally reduce overhead.

[phoddie]

Thanks for the report. It would be helpful to have a flow that shows the problem. Would you provide that?

[ralphwetzel]

Here is one:

[
    {
        "id": "4e803b9d8f9e4d3a",
        "type": "tab",
        "label": "Flow 6",
        "disabled": false,
        "info": "",
        "env": [],
        "_mcu": {
            "mcu": true
        }
    },
    {
        "id": "fb81498f180cccea",
        "type": "function",
        "z": "4e803b9d8f9e4d3a",
        "name": "null test",
        "func": "return [\n    { \"topic\": \"topic1\", \"payload\": 1 },\n    { \"topic\": \"topic2\", \"payload\": 2 },\n    null,\n    null,\n];",
        "outputs": 4,
        "noerr": 0,
        "initialize": "",
        "finalize": "",
        "libs": [],
        "_mcu": {
            "mcu": true
        },
        "x": 380,
        "y": 760,
        "wires": [
            [
                "5a24416334a0e289"
            ],
            [
                "5a24416334a0e289"
            ],
            [
                "5a24416334a0e289"
            ],
            [
                "5a24416334a0e289"
            ]
        ],
        "outputLabels": [
            "Delta [g/m3]",
            "Delta [%]",
            "Engage ventilation",
            "Stop ventilation"
        ]
    },
    {
        "id": "93cd1e5a4daf68e7",
        "type": "inject",
        "z": "4e803b9d8f9e4d3a",
        "name": "",
        "props": [
            {
                "p": "payload"
            },
            {
                "p": "topic",
                "vt": "str"
            }
        ],
        "repeat": "",
        "crontab": "",
        "once": false,
        "onceDelay": 0.1,
        "topic": "",
        "payload": "",
        "payloadType": "date",
        "_mcu": {
            "mcu": true
        },
        "x": 240,
        "y": 760,
        "wires": [
            [
                "fb81498f180cccea"
            ]
        ]
    },
    {
        "id": "5a24416334a0e289",
        "type": "debug",
        "z": "4e803b9d8f9e4d3a",
        "name": "debug 34",
        "active": true,
        "tosidebar": true,
        "console": false,
        "tostatus": false,
        "complete": "false",
        "statusVal": "",
        "statusType": "auto",
        "_mcu": {
            "mcu": true
        },
        "x": 560,
        "y": 760,
        "wires": []
    }
]

[phoddie]

Thank you for the example. That helps. I've committed a simple fix.

Regarding the implementation of send, it is important to keep in mind that the Function node has a unique requirement about setting the _msgid property (this took me a while to understand....). Rather than make the core send implementation more complex, the Function node implements what it needs and then passes control on to the core send. It is perhaps a bit more code than merging them, but it allows the core send to be faster and simpler for most messages.

FWIW – Node-RED itself takes a similar approach. The Function node there implements a sendResults function which passes over the outputs (mostly to set _msgid) and then calls its core send.

[ralphwetzel]

Thank you for the example. That helps. I've committed a simple fix.

Thank you! 👍

The Function node there implements a sendResults function which passes over the outputs (mostly to set _msgid) and then calls its core send.

Thank you for that reference. While I've checked some documentation, I got the impression that the msg handling in node-red-mcu is quite different to the core implementation, and (as it looks like) less efficient:

As the referenced document explains, core NR clones messages only in cases it is necessary; to achieve this, there's extra effort to be done in the FunctionNode. node-red-mcu yet clones every message (@ line 111): https://github.com/phoddie/node-red-mcu/blob/e00a3d7185bc61bf34d0b6259da9a0fbc668fd4c/nodered.js#L110-L121 Additionally, node-red-mcu seems to issue a new _msgid far more often than core NR.

I propose to invest additional time to validate both - to find/confirm the good fit for node-red-mcu.

[phoddie]

Thank you for confirming the fix!

Since the original issue is resolved, I'm going to close this issue out. Responses to the other points follow. Additional discussion on those should probably go in a separate issue or discussion topic.

The difference in the message cloning behavior is intentional. That is an optimization with potential far reaching side-effects that I've not yet had time to explore. The behavior now implemented is safe and can evolve over time.

...node-red-mcu seems to issue a new _msgid far more often than core NR.

A new _msgid is only generated when the message does not have one already:

m._msgid ??= generateId();