Messaging API support of core nodes #2402

k-toumura · 2019-12-09T14:48:17Z

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Proposed changes

Update core nodes to support new Node Messaging API.

Invocation of done() is deferred until the message has been removed from node's own message buffer/queue, or promise for the message processing has been fulfilled. For example, in delay node, done() is not called when a message has pushed to its queue. It is called after the message has been removed from the queue and sent to next nodes.

This PR is a preparation for Graceful Shutdown function, which needs to detect a completion of message processing.

Checklist

I have read the contribution guidelines
For non-bugfix PRs, I have discussed this change on the mailing list/slack team.
I have run grunt to verify the unit tests pass
I have added suitable unit tests to cover the new/changed functionality

coveralls · 2019-12-09T14:52:39Z

Coverage increased (+0.04%) to 76.898% when pulling dfa8b75 on node-red-hitachi:dev-messaging-api into 103e212 on node-red:dev.

knolleary · 2019-12-15T21:14:09Z

This is going to take quite some time to review given it changes so many core nodes.

In future, please consider breaking up items like this into multiple PRs that are easier to review in small steps.

dceejay · 2019-12-15T21:52:07Z

My basic comment is that they all seem to just replace
this.send(msg); with send(msg); - where send is just the passed in function....
What happens for all the cases where the node isn't called with the correct signature...
eg the inject not actually creates it's own events (on a timeout) - and calls node.emit("input", {});
so will it now fail ???
I think they all need to be protected with something like
send = send || function() { node.send.apply(node,arguments) };

k-toumura · 2019-12-16T00:19:51Z

This is going to take quite some time to review given it changes so many core nodes.
In future, please consider breaking up items like this into multiple PRs that are easier to review in small steps.

I agree. I'm sorry for causing you trouble.

What happens for all the cases where the node isn't called with the correct signature...
eg the inject not actually creates it's own events (on a timeout) - and calls node.emit("input", {});
so will it now fail ???
I think they all need to be protected with something like
send = send || function() { node.send.apply(node,arguments) };

From my understanding, we can safely assume that "input" callback is called with the correct signature, if we use a recent (1.0.0-beta3 or newer) runtime.
Node-RED runtime overrides native EventEmitter function in Node object. and node.emit("input", {}) calls Node.prototype.emit(), which overrides original EventEmitter.prototype.emit(). If the event is "input", this is just a wrapper for Node.prototoype._emitInput, and in that function it call a callback with proper signature.

knolleary · 2020-01-17T10:24:08Z

knolleary · 2020-05-29T21:11:04Z

packages/node_modules/@node-red/nodes/core/function/89-delay.js

-                }, delayvar);
+                    send(msg);
+                    done();
+                }, delayvar, () => done("queue cleared"));


I don't think clearing the delay queue should be considered an error - which calling done(...) with an argument suggests. The queue is being cleared because the user has asked for that to happen.

(This applies to all the places you call done(...) with a string argument).

ok. I'll remove an argument on done(...) when clearing the queue:

node-red/packages/node_modules/@node-red/nodes/core/function/89-delay.js

Line 124 in ee68fe0

}, node.timeout, () => done("queue cleared"));

node-red/packages/node_modules/@node-red/nodes/core/function/89-delay.js

Line 146 in ee68fe0

}, delayvar, () => done("queue cleared"));

node-red/packages/node_modules/@node-red/nodes/core/function/89-delay.js

Line 223 in ee68fe0

msgInfo.done("queue cleared");

node-red/packages/node_modules/@node-red/nodes/core/function/89-delay.js

Line 266 in ee68fe0

msgInfo.done("queue cleared");

node-red/packages/node_modules/@node-red/nodes/core/function/89-delay.js

Line 284 in ee68fe0

msgInfo.done("queue cleared");

packages/node_modules/@node-red/nodes/core/function/89-trigger.js

knolleary · 2020-05-29T21:37:29Z

packages/node_modules/@node-red/nodes/core/sequence/17-split.js

@@ -354,9 +360,12 @@ module.exports = function(RED) {
                node.pending_count = pending_count;
                var max_msgs = maxKeptMsgsCount(node);
                if ((max_msgs > 0) && (pending_count > max_msgs)) {
+                    Object.values(node.pending).forEach(group => {
+                        group.msgs.forEach(mInfo => mInfo.done(RED._("join.too-many")));


Can you describe the logic here?

It looks like you now report the too-many error against every single message, rather than just the most recent one. Not sure if that's the right behaviour - the error log will be flooded. But I think we need to discuss it.

My intention is to notify which message is dropped due to buffer overflow.
But, yes, it will cause a flood of error messages.

Possible alternative is:
- call msgInfo.done() for queued messages,
- call done(RED._("join.too-many")) for emit an error message, and
- call node.trace("overflowed message ["+msgInfo.msg.id+"]") to log the flashed messages.

Same reply for the Batch node - I agree with point 1 and 2 - but not 3 (node.trace)

I'll also update for this as same as Batch node.

packages/node_modules/@node-red/nodes/core/sequence/18-sort.js

knolleary · 2020-05-29T21:45:53Z

packages/node_modules/@node-red/nodes/core/sequence/19-batch.js

+                Object.values(node.pending).forEach(p_topic => {
+                    p_topic.groups.forEach(group => {
+                        group.msgs.forEach(msg => {
+                            msg.done("premature close");


Is this really an error? In other nodes the close function calls done() with no error even though the message will be discarded.

We really need to be consistent.

I forgot to comment to this. I removed all occurrences of "premature close" argument in batch node on commit c23e71b.

knolleary · 2020-05-29T21:47:26Z

packages/node_modules/@node-red/nodes/core/sequence/19-batch.js

+                Object.values(node.pending).forEach(p_topic => {
+                    Object.values(p_topic.groups).forEach(group => {
+                        group.msgs.forEach(msg => {
+                            msg.done(RED._("batch.too-many"))


As with other comments - is it really the right thing to throw an error against every message in the group? That is going to cause a lot of noise in the log.

as commented in split.js(#2402 (comment)), I propose following behaviour:

call msgInfo.done() for queued messages,

call done(RED._("batch.too-many")) for emit an error message, and

call node.trace("overflowed message ["+msgInfo.msg.id+"]") to log the flashed messages

I agree with calling done() on the queued messages and done(RED._("batch.too-many")) on the message that caused the overflow.

I do not think we should node.trace all of the flushed messages as again, that could be a huge list and there's little the user would be able to do with that information from the trace log.

I agree with this. I'll replace done(RED._("batch.too-many")) with done(), and emit a single error on the message that caused the overflow.

knolleary · 2020-09-23T09:22:52Z

We are almost there with this PR!

I think there is just the issue with the Batch/Join nodes on what they do when the max message count is reached. If we can get that sorted, then we can get this merged.

I do need to point out that the unit tests have not been updated for any of these changes. That is not ideal - especially as these are changes to such a core piece of functionality. I should have flagged that up when this PR was first raised.

Can you confirm you have manually tested all of the nodes covered by this PR for all the different paths that have been changed?

k-toumura · 2020-09-29T05:00:30Z

Can you confirm you have manually tested all of the nodes covered by this PR for all the different paths that have been changed?

I'll make unit tests to check the logic. Could you give me some more time?

k-toumura · 2020-10-05T11:50:45Z

At now, I'm looking delay nodes logic, and I found several oversights on calling done(), especially when a message is dropped.
I think that more comprehensive investigation is needed to avoid inconsistent behaviour.

Would you mind closing this PR?
And then, I'll create new PRs for each nodes (Delay, Trigger, CSV, Split, Join, Sort and Batch) with test cases (after 1.2 release)?

knolleary · 2020-10-09T16:13:48Z

This is your PR - if you think closing it and opening smaller individual PRs is a better choice, then that's absolutely fine by me 👍

k-toumura added 20 commits November 29, 2019 09:17

new-style callback function (inject node)

4619126

new-style callback function (complete node)

ad9d128

new-style callback function (catch node)

4ef481f

new-style callback function (status node)

3104aab

new-style callback function (link in/out node)

16547d6

new-style callback function (switch node)

fc6092f

new-style callback function (range node)

b9ed335

new-style callback function (template node)

0c189c0

new-style callback function (http response node)

dd530b8

new-style callback function (csv response node)

8763879

new-style callback function (html node)

7c8b3cf

new-style callback function (json node)

03baec0

new-style callback function (xml node)

0aff3d6

new-style callback function (yaml node)

0109f43

new-style callback function (batch node)

6b2e75f

new-style callback function (sort node)

233c807

new-style callback function (split/join node)

b16a35e

new-style callback function (trigger node)

007e483

new-style callback function (delay node)

ecf2ebd

readability fix

ee68fe0

knolleary added this to the 1.1 milestone May 21, 2020

knolleary reviewed May 29, 2020

View reviewed changes

packages/node_modules/@node-red/nodes/core/function/89-trigger.js Show resolved Hide resolved

knolleary reviewed May 29, 2020

View reviewed changes

packages/node_modules/@node-red/nodes/core/sequence/18-sort.js Show resolved Hide resolved

knolleary reviewed May 29, 2020

View reviewed changes

k-toumura added 4 commits June 1, 2020 12:21

avoid emitting error when clearing queue in delay and batch nodes

c23e71b

resolve conflict in merging dev branch

4ae773c

Merge remote-tracking branch 'origin/dev' into dev-messaging-api

7b62b70

Merge remote-tracking branch 'origin/dev' into dev-messaging-api

d492c21

knolleary modified the milestones: 1.1, 1.2 Jun 29, 2020

k-toumura mentioned this pull request Jul 14, 2020

Messaging API support of core nodes (cherrypicked) #2653

Merged

6 tasks

Merge branch 'dev' into dev-messaging-api

dfa8b75

knolleary closed this Oct 9, 2020

k-toumura mentioned this pull request Nov 5, 2020

Messaging API support in Trigger node #2751

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Messaging API support of core nodes #2402

Messaging API support of core nodes #2402

k-toumura commented Dec 9, 2019

coveralls commented Dec 9, 2019 •

edited

knolleary commented Dec 15, 2019 •

edited

dceejay commented Dec 15, 2019 •

edited

k-toumura commented Dec 16, 2019

knolleary commented Jan 17, 2020 •

edited

knolleary May 29, 2020

k-toumura Jun 1, 2020

knolleary May 29, 2020

k-toumura Jun 1, 2020 •

edited

knolleary Sep 23, 2020

k-toumura Sep 29, 2020

knolleary May 29, 2020

k-toumura Jun 2, 2020

knolleary May 29, 2020

k-toumura Jun 1, 2020 •

edited

knolleary Sep 23, 2020

k-toumura Sep 29, 2020

knolleary commented Sep 23, 2020

k-toumura commented Sep 29, 2020

k-toumura commented Oct 5, 2020

knolleary commented Oct 9, 2020

Messaging API support of core nodes #2402

Messaging API support of core nodes #2402

Conversation

k-toumura commented Dec 9, 2019

Proposed changes

Checklist

coveralls commented Dec 9, 2019 • edited

knolleary commented Dec 15, 2019 • edited

dceejay commented Dec 15, 2019 • edited

k-toumura commented Dec 16, 2019

knolleary commented Jan 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-toumura Jun 1, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-toumura Jun 1, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knolleary commented Sep 23, 2020

k-toumura commented Sep 29, 2020

k-toumura commented Oct 5, 2020

knolleary commented Oct 9, 2020

coveralls commented Dec 9, 2019 •

edited

knolleary commented Dec 15, 2019 •

edited

dceejay commented Dec 15, 2019 •

edited

knolleary commented Jan 17, 2020 •

edited

k-toumura Jun 1, 2020 •

edited

k-toumura Jun 1, 2020 •

edited