Incorrect done behavior when mixing streams of String and Buffer with Split node

[phoddie]

Current Behavior

Sending a mix of string and buffer payloads into a streaming split node results in what appears to be incorrect triggering of Complete nodes.

Expected Behavior

A Complete node should only be triggered for a message after it has been processed. In the case of a Split node, processed means that the input data has been fully output. In the current implementation, it is possible to receive a Complete trigger before the received data is output by the Split node.

Steps To Reproduce

Use the flow below to see the problem. The (condensed) output is

1. DONE (string "aa")
2. [0x61]
3. [0x62]
4. [0x62]
5. DONE (with buffer that generated 0x61, 0x62, and 0x63 output)
6. "aa"
7. DONE ??

Output 1 is unexpected as string "aa" hasn't been output until step 6.

The expected output is:

1. [0x61]
2. [0x62]
3. [0x62]
4. DONE (with buffer that generated 0x61, 0x62, and 0x63 output)
5. "aa"
6. DONE (with string that generated "aa")

Example flow

[
    {
        "id": "f1ef99ae46e3ab85",
        "type": "tab",
        "label": "split3-bug",
        "disabled": false,
        "info": "",
        "env": []
    },
    {
        "id": "a5fa4b17fa90dabf",
        "type": "inject",
        "z": "f1ef99ae46e3ab85",
        "name": "",
        "props": [
            {
                "p": "payload"
            }
        ],
        "repeat": "",
        "crontab": "",
        "once": true,
        "onceDelay": 0.1,
        "topic": "",
        "payload": "aa",
        "payloadType": "str",
        "x": 230,
        "y": 120,
        "wires": [
            [
                "576112f164d45ffb"
            ]
        ]
    },
    {
        "id": "0241766cbc77206f",
        "type": "inject",
        "z": "f1ef99ae46e3ab85",
        "name": "",
        "props": [
            {
                "p": "payload"
            }
        ],
        "repeat": "",
        "crontab": "",
        "once": true,
        "onceDelay": "0.2",
        "topic": "",
        "payload": "[97,33,98,33,99,33]",
        "payloadType": "bin",
        "x": 230,
        "y": 180,
        "wires": [
            [
                "576112f164d45ffb"
            ]
        ]
    },
    {
        "id": "584b48ac74ca1817",
        "type": "inject",
        "z": "f1ef99ae46e3ab85",
        "name": "",
        "props": [
            {
                "p": "payload"
            }
        ],
        "repeat": "",
        "crontab": "",
        "once": true,
        "onceDelay": "0.3",
        "topic": "",
        "payload": "!bb",
        "payloadType": "str",
        "x": 230,
        "y": 240,
        "wires": [
            [
                "576112f164d45ffb"
            ]
        ]
    },
    {
        "id": "576112f164d45ffb",
        "type": "split",
        "z": "f1ef99ae46e3ab85",
        "name": "",
        "splt": "!",
        "spltType": "str",
        "arraySplt": 1,
        "arraySpltType": "len",
        "stream": true,
        "addname": "",
        "x": 430,
        "y": 180,
        "wires": [
            [
                "9609730e293aa48b"
            ]
        ]
    },
    {
        "id": "9609730e293aa48b",
        "type": "debug",
        "z": "f1ef99ae46e3ab85",
        "name": "debug 10",
        "active": true,
        "tosidebar": true,
        "console": false,
        "tostatus": false,
        "complete": "true",
        "targetType": "full",
        "statusVal": "",
        "statusType": "auto",
        "x": 620,
        "y": 180,
        "wires": []
    },
    {
        "id": "f8ee960240d9a9f1",
        "type": "complete",
        "z": "f1ef99ae46e3ab85",
        "name": "",
        "scope": [
            "576112f164d45ffb"
        ],
        "uncaught": false,
        "x": 230,
        "y": 320,
        "wires": [
            [
                "4d5e8d6671f55485"
            ]
        ]
    },
    {
        "id": "4d5e8d6671f55485",
        "type": "change",
        "z": "f1ef99ae46e3ab85",
        "name": "",
        "rules": [
            {
                "t": "set",
                "p": "DONE",
                "pt": "msg",
                "to": "true",
                "tot": "bool"
            }
        ],
        "action": "",
        "property": "",
        "from": "",
        "to": "",
        "reg": false,
        "x": 420,
        "y": 320,
        "wires": [
            [
                "9609730e293aa48b"
            ]
        ]
    }
]

Environment

• Node-RED version: v3.0.2
• Node.js version: v19.2.0
• npm version: 8.19.3
• Platform/OS: Darwin 21.6.0 arm64 LE
• Browser: Chrome Version 108.0.5359.98

[knolleary]

The Split node has only ever been tested with streams of consistent types of payloads. It is quite uncommon to have a source of data that sometimes sends strings and sometimes sends buffers and to need to treat them as a single stream.

So I'm not sure how valid this issue is - or at least, how much of a priority it is to address.

That said, I did note in my comments on #4000 there is a different in behaviour of when done is called between String and Buffer handling... so maybe that's what we should focus on.

[phoddie]

So I'm not sure how valid this issue is - or at least, how much of a priority it is to address.

Understood.

It is quite uncommon to have a source of data that sometimes sends strings and sometimes sends buffers and to need to treat them as a single stream.

I'm sure! From your comment, I think you are saying that the intent is that there is one stream that can contain strings and buffers. But, a given instance sees only one or the other.

The implementation more-or-less assumes that there are two different input streams, one for strings and one for buffers. The state for each is mostly maintained separately. They overlap on the done handling, which is what causes the strange behaviors.

For the MCU Edition, to try to match that, I implemented it as two completely separate streams, including the done handling. That gives a very predictable behavior, at least. Here's my code, for reference. Notice that, like the full Node-RED code, it begins by checking the type of the input data.

When I first encountered the Split node, I thought it was odd that it wasn't configured for a particular type of input (buffer, stream, array, object) but instead carried the configuration for all of them. That can't be changed, of course.

It feels messy to try to support both strings and buffers in a single stream. Currently the output type is based on the input type: what would the output be on mixed input?

An easy compromise to implement could be to lock the stream to either buffer or string based on which it encounters first. If the other appears, that's an error. That would give predicable, consistent behavior. The risk is possible compatibility issues.