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

stream: ensure Stream.pipeline re-emits errors without callback #20437

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@phated
Contributor

phated commented Apr 30, 2018

Fixes an issue where Stream.pipeline wouldn't re-emit errors
on a stream if no callback was specified, thus swallowing
said errors.

Fixes: #20303

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

This comment has been minimized.

Contributor

phated commented Apr 30, 2018

@phated phated referenced this pull request Apr 30, 2018

Open

Cleanup before callback #12

// TODO: Should eos cleanup after itself?
// Ref https://github.com/mafintosh/end-of-stream/pull/12
cleanup();

This comment has been minimized.

@addaleax

addaleax Apr 30, 2018

Member

This seems to make sense, but I find it hard to tell how this part of the change relates to the rest. It feels like this should be something that comes with tests too?

This comment has been minimized.

@phated

phated Apr 30, 2018

Contributor

@addaleax so i stumbled around for ~2 days on this. Without cleaning up after end-of-stream, my re-emitted error was still being handled by the error handler that end-of-stream attached and I couldn't get an uncaughtException to be emitted. Once I added the cleanup, it emitted with no issues.

Would that go in this PR? A separate commit? I'm not sure the process here.

This comment has been minimized.

@addaleax

addaleax Apr 30, 2018

Member

Okay, makes sense. I think single PR and single commit is fine (but especially in the case of splitting into commits that correspond to logical changes you can use your own judgement). I think you can feel free to add a comment here that describes how this relates to the error handling, but this should be fine as-is.

This comment has been minimized.

@phated

phated Apr 30, 2018

Contributor

I've forced pushed an updated comment that describes what I've explained above.

@addaleax addaleax added the stream label Apr 30, 2018

@addaleax

This comment has been minimized.

Member

addaleax commented Apr 30, 2018

@mafintosh

This comment has been minimized.

Member

mafintosh commented May 4, 2018

Personally I'm 👎 on this. Emitting errors on other streams can have unintended side effects, which is why pipeline/pump tries not to do that in the first place. If all streams were "modern" you could call destroy(err) on them but thats not gonna behave well in streams < node 10 unless they have a custom destroy handler. Changing behavior based whether a cb is passed or not can lead to tricky bugs imo.

If people fell strongly about this I'd make the noop cb throw the error instead.

@addaleax

This comment has been minimized.

Member

addaleax commented May 4, 2018

@mafintosh I with @phated in that I think we should definitely do something that does not just swallow the errors silently…

@jasnell

This comment has been minimized.

Member

jasnell commented May 4, 2018

I agree with @mafintosh that this is likely not the best approach. Perhaps if pipeline() is not given a callback, a default non-op callback that throws when err is not null would be a better approach?

@mafintosh

This comment has been minimized.

Member

mafintosh commented May 4, 2018

@addaleax I wouldn't say errors are swallowed silently. On error the entire pipeline is shut down and destroy handlers are called on all streams. If you wanna be notified about this outside of your stream, you can pass a cb. Lots of use-cases of pump I've seen in the wild don't pass a cb, so if pipeline were to help them they'd have to pass in noop's.

I don't feel strongly though about that part (I do about the re-emitting errors part tho), as pump still exists and gonna keep that old behaivor.

@phated

This comment has been minimized.

Contributor

phated commented May 4, 2018

I'm fine if we avoid re-emitting the error and switch it to throwing in the noop - all I really care about is that the error is actually surfaced when there's no way to watch for it (not passing a callback).

Should I update the PR?

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented May 5, 2018

@jasnell if I understand your approach correct you want to do:

pipeline()

function pipeline(callback) {
  callback = callback || (err) => {
    if (err)
      throw err;
  }
  // Do stuff
}

To me this does not seem like a good approach since this will always end up as uncaught exception. Or did I misunderstand something?

I personally suggest to just force the user to always add a callback. If non is added, we just throw an error as we do in other APIs as well. If the user wants to ignore errors, that is totally fine.

@phated

This comment has been minimized.

Contributor

phated commented May 13, 2018

@jasnell @addaleax should I update the PR to rethrow in the noop callback?

@addaleax

This comment has been minimized.

Member

addaleax commented May 13, 2018

@phated I would do that, yes. 👍 It seems like that’s at least something that you, me, @jasnell and @mafintosh can be behind.

I personally suggest to just force the user to always add a callback.

@BridgeAR Also okay with me, but maybe for a follow-up PR since that’s tightening up behaviour even more?

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented May 18, 2018

@addaleax

but maybe for a follow-up PR since that’s tightening up behaviour even more

It does tighten the current rule but I highly prefer that over the current PR implementation. I think throwing async is a really bad idea. Using emit would be a way the user could deal with it properly but always triggering uncaught exceptions is 😕

@addaleax

This comment has been minimized.

Member

addaleax commented May 18, 2018

Using emit would be a way the user could deal with it properly but always triggering uncaught exceptions is 😕

I agree that generating uncaught exceptions is 😕, but it’s something for which users have a clear way to address.

More practically speaking, I think this PR could pass as not being semver-major (because there are cases where it’s known that no error is going to occur, and for the other cases this is effectively not silencing errors that should not be silenced, so a bug fix) whereas making the callback non-optional is definitely semver-major imo.

@jasnell

This comment has been minimized.

Member

jasnell commented May 23, 2018

Where are we at on this one? Definitely needs a rebase.

@phated

This comment has been minimized.

Contributor

phated commented May 23, 2018

This has been sitting for approval and feedback for a long time and I probably won't have time to rebase this coming up. Can someone else take that on?

@mcollina

This comment has been minimized.

Member

mcollina commented May 23, 2018

@mafintosh are you still -1 on this?

@mafintosh

This comment has been minimized.

Member

mafintosh commented May 24, 2018

@mcollina I think there are plenty of valid usecases where you don't want a cb because all error handling is contained in the pipeline (like a simple pipeline(fs.createReadStream(), httpRes)) so 👎 on that still.

I wouldn't block a merge though if people felt like it's super important.

This change https://github.com/nodejs/node/pull/20437/files#diff-eefad5e8051f1634964a3847b691ff84R49 should not be there IMO though as it makes error handling in practice tricky. With that change if a stream emits an error after end you'll have to setup your own error handling which in practice means you need to do that all the time rendering the API not that useful in many scenarios.

@mcollina

Marking my request of change explicit, so it does not get landed accidentally.

//
// TODO: Should eos cleanup after itself?
// Ref https://github.com/mafintosh/end-of-stream/pull/12
cleanup();

This comment has been minimized.

@mcollina

mcollina May 24, 2018

Member

I think this should not be done here. 'error' can still happen after 'end', making error handling more tricky.

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented May 29, 2018

@mafintosh as a general thing: you would be strongly against requiring a callback all the time? This is the default for pretty much everything async in Node.js and it would also solve this issue. Every user could easily just add a no-op callback in case they want to ignore that. Adding an explicit documentation about this should probably sufficient to explain this to users, don't you think so?

@mafintosh

This comment has been minimized.

Member

mafintosh commented May 29, 2018

@BridgeAR not strongly against it, just personal preference.

stream: ensure Stream.pipeline re-throws errors without callback
Fixes an issue where Stream.pipeline wouldn't re-throw errors
on a stream if no callback was specified, thus swallowing
said errors.

Fixes: #20303
@phated

This comment has been minimized.

Contributor

phated commented May 30, 2018

@addaleax @mcollina I've made rebased and made the desired changes. Can we get this wrapped up so I don't need to spend even more time rebasing and stuff?

@mcollina

LGTM

@mcollina

This comment has been minimized.

@mcollina

This comment has been minimized.

Member

mcollina commented May 30, 2018

This is ready to land with a green CI.

@mafintosh

This comment has been minimized.

Member

mafintosh commented May 30, 2018

👍

@addaleax

This comment has been minimized.

Member

addaleax commented May 31, 2018

Landed in c1012b4, thanks for the PR!

@addaleax addaleax closed this May 31, 2018

addaleax added a commit that referenced this pull request May 31, 2018

stream: ensure Stream.pipeline re-throws errors without callback
Fixes an issue where Stream.pipeline wouldn't re-throw errors
on a stream if no callback was specified, thus swallowing
said errors.

Fixes: #20303

PR-URL: #20437
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

addaleax added a commit that referenced this pull request May 31, 2018

stream: ensure Stream.pipeline re-throws errors without callback
Fixes an issue where Stream.pipeline wouldn't re-throw errors
on a stream if no callback was specified, thus swallowing
said errors.

Fixes: #20303

PR-URL: #20437
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

shisama added a commit to shisama/node that referenced this pull request Jun 5, 2018

stream: ensure Stream.pipeline re-throws errors without callback
Fixes an issue where Stream.pipeline wouldn't re-throw errors
on a stream if no callback was specified, thus swallowing
said errors.

Fixes: nodejs#20303

PR-URL: nodejs#20437
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

@MylesBorins MylesBorins referenced this pull request Jun 6, 2018

Merged

v10.4.0 proposal #21167

MylesBorins added a commit that referenced this pull request Jun 6, 2018

2018-06-06, Version 10.4.0 (Current)
Notable Changes:

* **deps**:
 - update V8 to 6.7.288.43 (Michaël Zasso)
   #19989
* **stream**:
  - ensure Stream.pipeline re-throws errors without callback (Blaine Bublitz)
    #20437

PR-URL: #21167

MylesBorins added a commit that referenced this pull request Jun 6, 2018

2018-06-06, Version 10.4.0 (Current)
Notable Changes:

* **deps**:
 - update V8 to 6.7.288.43 (Michaël Zasso)
   #19989
* **stream**:
  - ensure Stream.pipeline re-throws errors without callback (Blaine Bublitz)
    #20437

PR-URL: #21167

MylesBorins added a commit that referenced this pull request Jun 6, 2018

2018-06-06, Version 10.4.0 (Current)
Notable Changes:

* **deps**:
 - update V8 to 6.7.288.43 (Michaël Zasso)
   #19989
* **stream**:
  - ensure Stream.pipeline re-throws errors without callback (Blaine Bublitz)
    #20437

PR-URL: #21167

MylesBorins added a commit that referenced this pull request Jun 6, 2018

2018-06-06, Version 10.4.0 (Current)
Notable Changes:

* **deps**:
 - update V8 to 6.7.288.43 (Michaël Zasso)
   #19989
* **stream**:
  - ensure Stream.pipeline re-throws errors without callback (Blaine Bublitz)
    #20437

PR-URL: #21167

shisama added a commit to shisama/node that referenced this pull request Jun 9, 2018

stream: ensure Stream.pipeline re-throws errors without callback
Fixes an issue where Stream.pipeline wouldn't re-throw errors
on a stream if no callback was specified, thus swallowing
said errors.

Fixes: nodejs#20303

PR-URL: nodejs#20437
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

shisama added a commit to shisama/node that referenced this pull request Jun 9, 2018

2018-06-06, Version 10.4.0 (Current)
Notable Changes:

* **deps**:
 - update V8 to 6.7.288.43 (Michaël Zasso)
   nodejs#19989
* **stream**:
  - ensure Stream.pipeline re-throws errors without callback (Blaine Bublitz)
    nodejs#20437

PR-URL: nodejs#21167

shisama added a commit to shisama/node that referenced this pull request Jun 11, 2018

stream: ensure Stream.pipeline re-throws errors without callback
Fixes an issue where Stream.pipeline wouldn't re-throw errors
on a stream if no callback was specified, thus swallowing
said errors.

Fixes: nodejs#20303

PR-URL: nodejs#20437
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

shisama added a commit to shisama/node that referenced this pull request Jun 11, 2018

2018-06-06, Version 10.4.0 (Current)
Notable Changes:

* **deps**:
 - update V8 to 6.7.288.43 (Michaël Zasso)
   nodejs#19989
* **stream**:
  - ensure Stream.pipeline re-throws errors without callback (Blaine Bublitz)
    nodejs#20437

PR-URL: nodejs#21167

shisama added a commit to shisama/node that referenced this pull request Jun 12, 2018

stream: ensure Stream.pipeline re-throws errors without callback
Fixes an issue where Stream.pipeline wouldn't re-throw errors
on a stream if no callback was specified, thus swallowing
said errors.

Fixes: nodejs#20303

PR-URL: nodejs#20437
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

shisama added a commit to shisama/node that referenced this pull request Jun 12, 2018

2018-06-06, Version 10.4.0 (Current)
Notable Changes:

* **deps**:
 - update V8 to 6.7.288.43 (Michaël Zasso)
   nodejs#19989
* **stream**:
  - ensure Stream.pipeline re-throws errors without callback (Blaine Bublitz)
    nodejs#20437

PR-URL: nodejs#21167

shisama added a commit to shisama/node that referenced this pull request Jun 17, 2018

stream: ensure Stream.pipeline re-throws errors without callback
Fixes an issue where Stream.pipeline wouldn't re-throw errors
on a stream if no callback was specified, thus swallowing
said errors.

Fixes: nodejs#20303

PR-URL: nodejs#20437
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

shisama added a commit to shisama/node that referenced this pull request Jun 17, 2018

2018-06-06, Version 10.4.0 (Current)
Notable Changes:

* **deps**:
 - update V8 to 6.7.288.43 (Michaël Zasso)
   nodejs#19989
* **stream**:
  - ensure Stream.pipeline re-throws errors without callback (Blaine Bublitz)
    nodejs#20437

PR-URL: nodejs#21167
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment