Streams: forward errors on pipe #1521

calvinmetcalf · 2015-04-25T13:02:28Z

pull for nodejs/readable-stream#129 cc @iojs/streams , still need to add documentation, tests, and clean up the commits

chrisdickinson · 2015-04-25T22:51:05Z

lib/_stream_readable.js

@@ -447,6 +447,7 @@ Readable.prototype._read = function(n) {
 Readable.prototype.pipe = function(dest, pipeOpts) {


We should probably add this option to Stream.prototype.pipe as well.

Probably I forgot about that one

Adds an option for stream piping to enable errors to be pass along the pipe chain. Defaults it to false for it to be opt-in. This is an option to make our streams more in line with whatwg streams.

sonewman · 2015-04-27T22:03:40Z

lib/_stream_readable.js

@@ -447,6 +447,7 @@ Readable.prototype._read = function(n) {
 Readable.prototype.pipe = function(dest, pipeOpts) {
  var src = this;
  var state = this._readableState;
+  pipeOpts = pipeOpts || {};


should we care that in a typical use-case this would create an arbitrary object?

we also avoid an unnecessary conversion of undefined to a Boolean, I doubt it makes much difference

rvagg · 2015-04-28T04:07:43Z

Can this be easily detected if it's set on a stream? Can I !!stream.options.forwardErrors or similar?

Also be aware that this will screw up a lot of user code that assumes that errors aren't forwarded, for example in bl which is in heavy use.

And another thing:

If the stream should pass errors down the pipe instead of throwing them.

My reading of the code says that this isn't quite accurate - errors aren't thrown if they are handled and my assumption when reading this description is that they are only forwarded if they were going to throw, i.e. if there was no other error handler. Perhaps tighten that description up a bit more.

Overall I'm weary of this, I'd love for this to have been the default behaviour of streams from the beginning but we've built a whole ecosystem around the assumption that streams don't forward errors and you need to handle them all the way down; as crappy as that is.

chrisdickinson · 2015-04-28T07:42:00Z

@rvagg:

Can this be easily detected if it's set on a stream? Can I !!stream.options.forwardErrors or similar?

No. This is an option set on the pipe relationship between two streams, not on a stream itself. There's no user-accessible backing object describing that relationship, so it's not queryable.

Another concern: streams that don't support this option in pipe don't alert the user that the option isn't supported. So, if someone's using a v0.10 readable-stream (or v0.12 readable-stream) they can call oldStream.pipe(newStream, {forwardErrors: true}) and it will, at the outset, look well-configured. However, the old stream won't actually forward errors, and I imagine tracking down that idiosyncracy will be difficult.

calvinmetcalf · 2015-04-28T09:32:10Z

Also be aware that this will screw up a lot of user code that assumes that errors aren't forwarded, for example in bl which is in heavy use.

indeed this is why it's opt in

My reading of the code says that this isn't quite accurate - errors aren't thrown if they are handled and my assumption when reading this description is that they are only forwarded if they were going to throw, i.e. if there was no other error handler. Perhaps tighten that description up a bit more.

will do

Another concern: streams that don't support this option in pipe don't alert the user that the option isn't supported. So, if someone's using a v0.10 readable-stream (or v0.12 readable-stream) they can call oldStream.pipe(newStream, {forwardErrors: true}) and it will, at the outset, look well-configured. However, the old stream won't actually forward errors, and I imagine tracking down that idiosyncracy will be difficult.

we could put an attribute on the new stream to say it supports the option

sonewman · 2015-04-29T20:07:15Z

it seems a little confusing. Essentially as a feature this would be advantage to have, and brings parity to WHATWG. Is the fact that it could lead mislead an user a game changer at this stage? I assume a lot of changes could also warrant these kinds of developer confusion. It would be important for changes like this to be well advertised.

I can't think how this could otherwise be implemented without introducing this extra complexity 😕💭

calvinmetcalf · 2015-04-29T21:00:13Z

We could give it a different method name like the equivalent whatwg stream
name pipeThrough, except maybe shorter and user friendly

On Wed, Apr 29, 2015, 4:08 PM Sam Newman [email protected] wrote:

it seems a little confusing. Essentially as a feature this would be
advantage to have, and brings parity to WHATWG. Is the fact that it could
lead mislead an user a game changer at this stage? I assume a lot of
changes could also warrant these kinds of developer confusion. It would be
important for changes like this to be well advertised.

I can't think how this could otherwise be implemented without introducing
this extra complexity [image: 😕][image: 💭]

—
Reply to this email directly or view it on GitHub
#1521 (comment).

domenic · 2015-04-29T21:02:28Z

@ForbesLindesay's prior art calls it syphon; seems reasonable.

calvinmetcalf · 2015-04-29T21:49:22Z

@ForbesLindesay's prior art calls it syphon; seems reasonable.

I like it, syphon as alias for pipe that adds the forward error thingy

just-boris · 2015-05-27T21:48:09Z

This is a nice feature! It makes work with streams more consistent with promises:

getStream()
    .pipe()
    .pipe()
    .pipe()
    .on('error', onError)

and the same thing with promises

getPromise()
    .then()
    .then()
    .then()
    .catch(onError)

It's nice to have one error handler at the end of chain instead of catching error after every operation

jasnell · 2015-10-22T02:31:14Z

@calvinmetcalf @chrisdickinson @trevnorris @rvagg ... what's the status on this one?

calvinmetcalf · 2015-10-22T09:49:21Z

@jasnell for know I'd use pump

rvagg · 2015-10-25T21:39:24Z

I'm torn on this one, I wish it was the default but the ship has sailed. A possible compromise might be for the behaviour to be that it'll forward errors if there are no existing error listeners on the current stream, although that creates weird state and would likely go against most users' expectations.

calvinmetcalf · 2015-11-05T16:29:45Z

@rvagg I like that I'll update the patch to do just that

calvinmetcalf · 2015-11-11T15:28:55Z

ok so thinking through how only forward if there are no listeners, that is going to be tricky if it's part of a pipe because currently the stream that is piping to the stream has a listener and ends up throwing the error if nobody else is

Qard · 2015-11-12T19:41:38Z

Why not just make a new function that does that automatically, with the intent of eventually deprecating the current pipe function? (Obviously on a very long deprecation cycle, since streams are everywhere.)

It's fine to fear breaking things, but I think we might fear changing things a bit too much at times. With time, we can steer the community toward newer and better ways.

denis-sokolov · 2015-12-09T21:59:29Z

@calvinmetcalf, do you mind stating in a sentence or two the resolution of this?

mscdex added the stream Issues and PRs related to the stream subsystem. label Apr 25, 2015

chrisdickinson reviewed Apr 25, 2015
View reviewed changes

Streams: option to forward errors on pipe

b537275

Adds an option for stream piping to enable errors to be pass along the pipe chain. Defaults it to false for it to be opt-in. This is an option to make our streams more in line with whatwg streams.

calvinmetcalf force-pushed the stream-forward-error branch from c793225 to b537275 Compare April 27, 2015 16:11

sonewman reviewed Apr 27, 2015
View reviewed changes

ivogabe mentioned this pull request May 28, 2015

Gulpfile improvements for task build.tools angular/angular#2209

Closed

bnoordhuis force-pushed the master branch from 311c3e4 to b926718 Compare June 2, 2015 17:25

rvagg force-pushed the master branch from 9d21135 to 628a3ab Compare June 25, 2015 09:18

indutny force-pushed the master branch from bffb204 to eb35968 Compare July 22, 2015 21:21

rvagg force-pushed the master branch from 11c25c2 to ba02bd0 Compare September 6, 2015 11:55

calvinmetcalf mentioned this pull request Nov 11, 2015

add error forwarding nodejs/readable-stream#129

Closed

calvinmetcalf closed this Dec 9, 2015

Fishrock123 mentioned this pull request Apr 8, 2016

Proposal for .pipe() - forward error emitted from src to dest in stream #6113

Closed

dvlsg mentioned this pull request Nov 7, 2017

A different approach to filtering? dvlsg/async-csp#17

Open

snyk-bot mentioned this pull request Jul 28, 2021

[Snyk] Upgrade commander from 2.11.0 to 8.0.0 MaxMood96/node#4

Open

aliscco mentioned this pull request Apr 25, 2023

[Snyk] Fix for 19 vulnerabilities aliscco/alisco-node#172

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Streams: forward errors on pipe #1521

Streams: forward errors on pipe #1521

calvinmetcalf commented Apr 25, 2015

chrisdickinson Apr 25, 2015

calvinmetcalf Apr 26, 2015

sonewman Apr 27, 2015

calvinmetcalf Apr 27, 2015

rvagg commented Apr 28, 2015

chrisdickinson commented Apr 28, 2015

calvinmetcalf commented Apr 28, 2015

sonewman commented Apr 29, 2015

calvinmetcalf commented Apr 29, 2015

domenic commented Apr 29, 2015

calvinmetcalf commented Apr 29, 2015

just-boris commented May 27, 2015

jasnell commented Oct 22, 2015

calvinmetcalf commented Oct 22, 2015

rvagg commented Oct 25, 2015

calvinmetcalf commented Nov 5, 2015

calvinmetcalf commented Nov 11, 2015

Qard commented Nov 12, 2015

denis-sokolov commented Dec 9, 2015

		@@ -447,6 +447,7 @@ Readable.prototype._read = function(n) {
		Readable.prototype.pipe = function(dest, pipeOpts) {

Streams: forward errors on pipe #1521

Streams: forward errors on pipe #1521

Conversation

calvinmetcalf commented Apr 25, 2015

chrisdickinson Apr 25, 2015

Choose a reason for hiding this comment

calvinmetcalf Apr 26, 2015

Choose a reason for hiding this comment

sonewman Apr 27, 2015

Choose a reason for hiding this comment

calvinmetcalf Apr 27, 2015

Choose a reason for hiding this comment

rvagg commented Apr 28, 2015

chrisdickinson commented Apr 28, 2015

calvinmetcalf commented Apr 28, 2015

sonewman commented Apr 29, 2015

calvinmetcalf commented Apr 29, 2015

domenic commented Apr 29, 2015

calvinmetcalf commented Apr 29, 2015

just-boris commented May 27, 2015

jasnell commented Oct 22, 2015

calvinmetcalf commented Oct 22, 2015

rvagg commented Oct 25, 2015

calvinmetcalf commented Nov 5, 2015

calvinmetcalf commented Nov 11, 2015

Qard commented Nov 12, 2015

denis-sokolov commented Dec 9, 2015