node-red / node-red

Low-code programming for event-driven applications
http://nodered.org
Apache License 2.0
19.37k stars 3.36k forks source link

msg.req cannot be cloned if moved to another property name #1367

Open btsimonh opened 7 years ago

btsimonh commented 7 years ago

this seems to be related to https://github.com/node-red/node-red/issues/97

If an output of an http request node is split, and the message contains a reference to a msg output from a http-in node, the msg cloning causes an exception which is uncaught, and leaves the flow in a bit of a limbo state. If the split is removed the flow works. If the split is done after a function node, the exception is caught and reported in NR on the function node (i.e. where the clone occurs).

This issue is about the http-request node not catching the exception and getting into a bad state (stays at requesting). There may be other nodes which are affected by exceptions in clone.... The state may get worse if it's an https endpoint or behind HAProxy (got nasty NR very busy scenarios which I could not explain, but which did not occur with direct http access to the NR endpoint).

The exception is reported in the console as: NodeRedProcess[2799]: 10 Aug 08:59:56 - [red] Uncaught Exception: NodeRedProcess[2799]: 10 Aug 08:59:56 - TypeError: Cannot set property listening of # which has only a getter NodeRedProcess[2799]: at _clone (/home/simon/tmp/src/nr/nodered/node_modules/node-red/node_modules/clone/clone.js:156:16) NodeRedProcess[2799]: at _clone (/home/simon/tmp/src/nr/nodered/node_modules/node-red/node_modules/clone/clone.js:156:18) NodeRedProcess[2799]: message repeated 3 times: [ at _clone (/home/simon/tmp/src/nr/nodered/node_modules/node-red/node_modules/clone/clone.js:156:18)] NodeRedProcess[2799]: at clone (/home/simon/tmp/src/nr/nodered/node_modules/node-red/node_modules/clone/clone.js:196:10) NodeRedProcess[2799]: at Object.cloneMessage (/home/simon/tmp/src/nr/nodered/node_modules/node-red/red/runtime/util.js:53:13) NodeRedProcess[2799]: at HTTPRequest.Node.send (/home/simon/tmp/src/nr/nodered/node_modules/node-red/red/runtime/nodes/Node.js:177:61) NodeRedProcess[2799]: at IncomingMessage. (/home/simon/tmp/src/nr/nodered/node_modules/node-red/nodes/core/io/21-httprequest.js:263:30) NodeRedProcess[2799]: at emitNone (events.js:91:20)

reproducing flow:

[{"id":"5b7938f8.be0ca8","type":"function","z":"bd398b8b.d9f498","name":"prepareget","func":"\nnode.warn(msg);\n\nvar getgoogle = function(){\n // create a rest call to read the instance\n var newmsg = {};\n newmsg.method = 'GET';\n newmsg.url = 'https://www.google.com';\n newmsg.originalmsg = msg;\n node.warn(\"getgoogle()\");\n node.send([null, newmsg]);\n return;\n}\n\n\n\n\nif (msg.payload){\n getgoogle();\n return;\n}\n\nvar newmsg = {\n payload: 'no payload'\n};\n\nnode.send([newmsg, null]);\n\n\nreturn;","outputs":"2","noerr":0,"x":170,"y":200,"wires":[["4b3eccfd.a1c7d4"],["7276f617.7da5f8"]]},{"id":"7276f617.7da5f8","type":"http request","z":"bd398b8b.d9f498","name":"","method":"use","ret":"txt","url":"","tls":"","x":350,"y":220,"wires":[["4438514a.90f06","949f1af7.9e1f48"]]},{"id":"949f1af7.9e1f48","type":"debug","z":"bd398b8b.d9f498","name":"","active":true,"console":"false","complete":"true","x":710,"y":240,"wires":[]},{"id":"f4bbb89f.36d168","type":"http in","z":"bd398b8b.d9f498","name":"/image/test","url":"/image/test","method":"get","upload":false,"swaggerDoc":"","x":120,"y":120,"wires":[["5b7938f8.be0ca8"]]},{"id":"ab6c86ae.7b4248","type":"http response","z":"bd398b8b.d9f498","name":"","x":790,"y":320,"wires":[]},{"id":"4b3eccfd.a1c7d4","type":"debug","z":"bd398b8b.d9f498","name":"","active":true,"console":"false","complete":"true","x":530,"y":100,"wires":[]},{"id":"9d58645c.24f698","type":"inject","z":"bd398b8b.d9f498","name":"","topic":"","payload":"","payloadType":"date","repeat":"","crontab":"","once":false,"x":120,"y":260,"wires":[["5b7938f8.be0ca8"]]},{"id":"4438514a.90f06","type":"function","z":"bd398b8b.d9f498","name":"returnresults","func":"\nnode.warn(\"gothere\");\n\n// restore the original request msg\nvar newmsg = msg.originalmsg;\n\n\nif (msg.statusCode !== 200){\n newmsg.statusCode = msg.statusCode;\n newmsg.payload = { error: \"could not read google\" };\n return newmsg;\n}\n\n\nif (msg.payload){\n newmsg.payload = msg.payload;\n newmsg.statusCode = 200;\n return newmsg;\n}\n\n\nnewmsg.statusCode = 500;\nnewmsg.payload = { error: \"could not read google 2\" };\nreturn newmsg;\n","outputs":1,"noerr":0,"x":470,"y":320,"wires":[["949f1af7.9e1f48","ab6c86ae.7b4248"]]}]

knolleary commented 7 years ago

We don't support copying msg.res to other message properties for exactly this. It needs special handling we can't apply generically to every property.

btsimonh commented 7 years ago

Ahhh.. I've been doing it wrong all this time. msg.res will pass though the http-request node; I had sort of assumed it would not for some reason. Will modify all my flows accordingly.

But it would still be nice if the failure was highlighted; maybe either any exception in clone should be pushed to the debug window (with a note saying 'watch out for items which can't be cloned'), or if an exception occurs trying to clone, that the message should be duplicated un-cloned (which for MOST flows would be an ok fallback from my simplistic perspective);

I know this is a complex issue,

tschleuss commented 6 years ago

Guys, I'm having a similar problem with that exception

node-red_1     | 30 Aug 21:21:50 - [red] Uncaught Exception:
node-red_1     | 30 Aug 21:21:50 - TypeError: Cannot set property listening of #<Server> which has only a getter
node-red_1     |     at _clone (/usr/src/node-red/node_modules/clone/clone.js:156:16)
node-red_1     |     at _clone (/usr/src/node-red/node_modules/clone/clone.js:156:18)
node-red_1     |     at _clone (/usr/src/node-red/node_modules/clone/clone.js:156:18)
node-red_1     |     at _clone (/usr/src/node-red/node_modules/clone/clone.js:156:18)
node-red_1     |     at _clone (/usr/src/node-red/node_modules/clone/clone.js:156:18)
node-red_1     |     at clone (/usr/src/node-red/node_modules/clone/clone.js:196:10)
node-red_1     |     at Object.cloneMessage (/usr/src/node-red/node_modules/node-red/red/runtime/util.js:53:13)
node-red_1     |     at /usr/src/node-red/node_modules/node-red/red/runtime/nodes/flows/Flow.js:262:48
node-red_1     |     at Array.forEach (<anonymous>)
node-red_1     |     at Flow.handleError (/usr/src/node-red/node_modules/node-red/red/runtime/nodes/flows/Flow.js:256:34)

I have a list of data that I need to POST to a webservice. I'm developing my own node, and everything works fine until I fire my HTTP post. Basically, I have this snippet of code

            const objects = msg.payload || []
            const hpptCalls = objects.map(obj => (callback) => {
                processObject(obj, callback)
            })
            async.parallelLimit(hpptCalls, 5, (err, results) => {
                node.send(msg)
            })

Inside my processObject function, I have almost the same code that exists in core node 21-httprequest.js


            const req = http.request(options, res => {
                let data = ''
                res.on('data', chunk => {
                    data += chunk
                })
                res.on('end', () => {
                    callback(null)
                })
            })

            req.setTimeout(120000, () => {
                node.error('Error', msg)
                req.abort()
                callback('Error')
            })

            req.on('error', err => {
                node.error(err, msg)
                callback(err)
            })

            if (content) {
                req.write(JSON.stringify(content))
            }

            req.end()

But, it seems that when the first HTTP post is called, my node-red crash entirely with that exception. I can figure what is the problem. Do you figure out what is causing that problem already?

tschleuss commented 6 years ago

Apparently found the problem, as you mention above. At a random point in the flow, someone put a function node with the following code

return {
   "_msg": msg
   [other properties]
}

Thanks! (It was hard to find the problem lol, we have a big flow).