fix: prevent stale error handler references in file node append mode#5460
fix: prevent stale error handler references in file node append mode#5460Dennis-SEG wants to merge 2 commits intonode-red:masterfrom
Conversation
knolleary
left a comment
knolleary
left a comment
There was a problem hiding this comment.
The original issue says the error handler is setup once and references the original done and msg objects.
With this PR, that error handler is still in place in line 186; it is still registering the handler just as it did before.
For the proposed changes, the docs for stream.write (ref) say the callback is called before the error event is emitted. I can't see how the logic around writeDone will work.
It's also worth noting that the callback will be passed any error as the first argument (again, before the error event is emitted). This could be used to call the right scoped nodeSend/done.
Use write callback's error argument instead of adding/removing error handlers. This ensures the correct msg/done scope is used for each write operation. The stream-level error handler now only handles errors for dynamic filenames, while static filename writes handle errors via the write callback. Addresses review feedback from @knolleary. Fixes node-red#5453
Dennis-SEG
force-pushed
the
fix/file-node-error-handler
branch
from
091f344 to
4822636
Compare
knolleary
left a comment
knolleary
left a comment
There was a problem hiding this comment.
Need to think through the potential impact of changing behaviour here.
| if (err) { | ||
| node.error(RED._("file.errors.appendfail",{error:err.toString()}),msg); | ||
| } else { | ||
| nodeSend(msg); | ||
| } |
There was a problem hiding this comment.
I think this will result in a change in behaviour. Previously, it would still do a nodeSend regardless of the error. Now, it will halt the flow in the case of an error.
It's hard to judge if that's okay to change as a fix, rather than a breaking change - we don't know how often this scenario is being hit as we've had no reports on it.
|
I've updated the fix based on your suggestion to use the write callback's error argument. Here's the approach: Static filenames: Use write callback with error argument node.wstream.write(buf, function(err) {
if (err) {
node.error(..., msg); // correct msg scope
} else {
nodeSend(msg);
}
done(); // correct done scope
});Dynamic filenames: Keep using error event (safe because new stream per message)
Error event handler: Now checks node.wstream.on("error", function(err) {
if (node.filenameType !== "str" && node.filenameType !== "env") {
node.error(..., msg);
done();
}
});I verified with Node.js tests that the write callback is called before the error event (as the docs state), so for static filenames:
This ensures |
2 participants
Summary
Fixes #5453 - File node's append mode could have stale error handler references when stream is reused.
Changes
Added per-write error handling with a
writeDoneflag to ensure correct message context is used and prevent doubledone()calls.Test Plan