-
Notifications
You must be signed in to change notification settings - Fork 30.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pipeline error source #34873
Pipeline error source #34873
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not like this PR, If you want to debug which pipeline crashed, you could add debug log in this module, instead of adding this unnecessary fields.
like
Lines 42 to 44 in 22e3ada
let debug = require('internal/util/debuglog').debuglog('http', (fn) => { | |
debug = fn; | |
}); |
Line 521 in 22e3ada
debug('parse error', ret); |
@@ -181,6 +182,8 @@ function pipeline(...streams) { | |||
const reading = i < streams.length - 1; | |||
const writing = i > 0; | |||
|
|||
finish.bind({ index: i }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function.prototype.bind doesn't modify the original finish
function, so that line has no effect as is. You would need to do something like:
const finishWithIndex = finish.bind({ index: i });
// ... and below, change occurrences of `finish` in the function scope
destroys.push(destroyer(ret, false, true, finishWithIndex));
Not a very elegant variable name, I'm very bad at naming those things 😅
ping @nodejs/streams |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are failing.
Not sure I see the need for this? You can just attach an 'error'
handler to each stream you pass into pipeline?
Not debugging, I think what makes sense is that you can make a corresponding downgrade strategy based on which stream is in error. |
Closing this as stale. |
Fixes: #34842
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes (not needed)added a property index to error object in the pipeline function to that point towards the error causing stream