Skip to content
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

Closed
wants to merge 3 commits into from
Closed

Pipeline error source #34873

wants to merge 3 commits into from

Conversation

Deepp0925
Copy link

Fixes: #34842

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes (not needed)
  • tests and/or benchmarks are included (not needed)
  • documentation is changed or added
  • commit message follows commit guidelines

added a property index to error object in the pipeline function to that point towards the error causing stream

@Deepp0925 Deepp0925 mentioned this pull request Aug 22, 2020
4 tasks
Copy link
Member

@himself65 himself65 left a 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

node/lib/_http_common.js

Lines 42 to 44 in 22e3ada

let debug = require('internal/util/debuglog').debuglog('http', (fn) => {
debug = fn;
});
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 });
Copy link
Contributor

@aduh95 aduh95 Aug 24, 2020

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 😅

@rickyes
Copy link
Contributor

rickyes commented Aug 24, 2020

ping @nodejs/streams

Copy link
Member

@ronag ronag left a 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?

@rickyes
Copy link
Contributor

rickyes commented Aug 24, 2020

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

node/lib/_http_common.js

Lines 42 to 44 in 22e3ada

let debug = require('internal/util/debuglog').debuglog('http', (fn) => {
debug = fn;
});

debug('parse error', ret);

Not debugging, I think what makes sense is that you can make a corresponding downgrade strategy based on which stream is in error.

@targos targos added the stream Issues and PRs related to the stream subsystem. label Dec 27, 2020
@ronag
Copy link
Member

ronag commented Dec 27, 2020

Closing this as stale.

@ronag ronag closed this Dec 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pipeline error causing stream
6 participants