stream: pipeline uncaught error

[ronag]

There are stream(like)s both in node and ecosystem that can emit an error after finished/eos. This will cause the process to unexpectedly exit with an uncaught error exception.

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

[Fishrock123]

That sounds like an implementation problem in a userland stream. I don't know that we should cover that at risk of obscuring valid problems?

[ronag]

We have it in native streams as well...

[ronag]

The rule (as far as I understand) is that streams can emit errors all the way until ‘close’. ‘eos’ finishes before ‘close’.

[ronag]

There is actually already a bug in eos where it doesn’t remove its error listener. So this PR doesn’t actually change any behavior until eos is fixed.

[Trott]

@ronag Should this remain open? If so, is it blocked by something else?

[ronag]

It should remain open and is not blocked

[Trott]

/ping @nodejs/streams

[mcollina]

This needs a unit test

[mafintosh]

That eos does not remove the error listener is not a bug but a feature to handle exactly this problem

[ronag]

That eos does not remove the error listener is not a bug but a feature to handle exactly this problem

That seems a little random? Shouldn't "the problem" be handled as local as possible? Currently, using finished could unexpectedly swallow errors?

@lpinca @mcollina

[ronag]

@mcollina a unit test here won't really help since eos (incorrectly?) leaves a dangling 'error' handler. Until that is fixed this change is pretty much a noop. How should we/I handle that?

[mcollina]

I’m -1. This is handled by eos already, in fact this change has to disable it.

I don’t understand why this is worth considering.

[ronag]

@mcollina Updated with unit test.

This is handled by eos already, in fact this change has to disable it.

It is "magical" and also undocumented that eos handles it. I'm also of the opinion that eos should not leave dangling listeners once it has completed, i.e. currently (outside of pipeline use) it can unexpectedly swallow errors. However, that can not be "fixed"/changed until something like this PR.

If this PR is rejected I would at least suggest we update the documentation for eos/finished.

[mcollina]

I think the correct approach is to document it in eos.

[ronag]

Sure, @lpinca do you have any opinion or should I just close this?

[ronag]

@mcollina I guess you are -1 on this as well then #28820?

[mcollina]

Yes, exactly.

[ronag]

New PR with documentation change: #28935