any error thrown by mapper should be emitted

[stevemao]

Ref: https://github.com/dominictarr/split/blob/master/index.js#L27-L32

[mafintosh]

even though this behaviour was in the original split this sounds like a really bad idea to me that's gonna introduce weirds bugs / side effects that'll be impossible to find

stuff like

var badStream = split(function (line) {
  return lone.toString()
})

Will result in a reference error magically being emitted as an error. Combined with something like pump

pump(badStream, ..., function (err) {
 // err might be a reference error or some other very unattended thing
 // aka - your program might be in a very inconsistent state now
 // and its incredible hard to tell if this was an ref/conf error or an actual error
})

Use an explicit try-catch around the sensitive code instead and handle the error accordingly.

[stevemao]

So where can i find the reference that a reference error should not be emitted as an error?

[mafintosh]

https://www.joyent.com/developers/node/design/errors

see the "Operational errors vs. programmer errors" section

[mcollina]

I completely agree with mafintosh. In fact, there is very little reference on this, but all the experience we had in working with streams and node. I did not even knew about through original behavior: I will update the README.

[stevemao]

This is pretty scary... I have been reading many articles and I still don't understand how stream handles errors. I have raised an issue nodejs/node#4491 about throwing in stream. it's just too tricky. I'm feeling it's way too hard to understand how streams really work.

I like the promise API

promise
.catch()

And it just catches everything.

[stevemao]

Not too mention there is too little information on this...

We seriously need a complete spec and guide on stream.

[mcollina]

@stevemao this is great feedback in "how streams really work".

However this is a stream library, so it complies with that world. Also, complaining about this here is not helping solving your problem, it usually alienates the maintainers of a library. Take lead in solving this problem, as it mostly is about docs.

[mafintosh]

@stevemao the promise api has exactly this problem as well though because of the magic try-catch. no way to know if an exception was a fatal exception of a runtime exception. this can lead to bad memory leaks in a server environment so be careful :)

[stevemao]

Don't wanna complain :) I have done serious researches. Especially the problems that gulp has. I have followed a few issues there because I have similar problems (but theirs are way more complicated than me). Even @dominictarr made this mistake. I'm still kinda confused now. Let me take some time to think through :)

@mafintosh If you could write an article about this I think a lot of people will appreciate :) I can't think of any real life example of the problems you describe.

[mafintosh]

This isn't a problem with streams honestly. This is mostly a problem with magic error handling.
Your program can enter some weird states when you implicitly try-catch everything.

Consider this example

var pool = [stream1, stream2, stream3]

...

somePromise.then(function () {
  // lets cycle the streams so we spread out the load
  var s = pool.shift()
  s.writee('hello') // oops i made a typo
  pool.push(s)

  return someAnotherPromise
})

Now running this code will make the promise chain call catch

promise.catch(function (err) {
  // err is a my typo error!
})

But our program is now in a weird state because my stream was never re-added to the pool of streams (that stream might result in a leak now) and if I ran that promise code 4 times the pool would be magically empty on the 4th run (because errors happened in the past!).

This leaves my program in inconsistent state that I didn't intent. The better solution here would been to let my program crash so I would see that there was a typo error so I could go back in fix it.

[mafintosh]

This becomes even more tricky if you we're using a third party library that accidentally threw instead of a simple typo error.

[stevemao]

Thanks @mafintosh for writing this up. It makes perfect sense. I have actually seen the article you have linked. I have done a bit more research and found out that even the most famous JavaScript developers have completely opposite view on the error handling (I don't strongly support or oppose any party). So I think we should stick to the spec (which is pretty much undocumented at the moment). And yes, you guys are right about the stream behaviour here :)

[chrisdickinson]

I'm coming in from nodejs/readable-stream#181 — I can butt out if this isn't useful feedback!

I tend to lean towards the opposite view, here: that is, I believe if a stream encounters an error during processing, it should surface that error as an "error" event, so that the specific stream instance causing the error can be identified (vs. the "class" of stream that caused the error.) Since the error thrown would generate a stack that wouldn't point to any stream instance in particular, it can be hard to track such errors down to an actionable location in your code to fix. If there are no error listeners for the stream, the code will still crash.

It seems to me that typo-as-programmer-error issues should be caught by test suites or linters, not necessarily by program crashes + post-mortem debugging sessions. Even in cases where an error is thrown, there's no guarantee it will crash the program — calling code always has the option to resume, for better or worse, so it seems like making stream's behavior with regards to error propagation regular would be of high value to users.

[yoshuawuyts]

it should surface that error as an "error" event

This would confound (unexpected) programmer errors with (expected) runtime errors - they should not be munged together. If stack traces aren't clear, then that should be addressed instead; combining both types of errors would lead to unnecessary complexity, I think we should stay far away from that.

[stevemao]

I found two tweets that's quite interesting
https://twitter.com/domenic/status/695339493033844737
https://twitter.com/mafintosh/status/695278565055709185

[spion]

Not really related to this PR, but I'd like to explain why this isn't about "magic error handling". Consider:

var s = pool.shift();
// lots of code
doSomeOperation((err, res) =>
  if (err) return done(err)
  // ...
  // lots of more code
  // ...
  s.write(res)
  pool.push(s)
})

There is no magic error handling, but its easy to miss that we're not releasing the resource properly there.

This is all about node not having a proper resource management strategy or API. And no, crashing the process isn't one.

Promises would indeed have a problem if you use them that way. However they make it really easy to build a mechanism to safely use resources:

Bluebird example:

function getStream(pool) {
  return Promise.resolve(pool.shift()).disposer(s => pool.push(s));
}

somePromise.then(_ => Promise.using(getStream(pool), s => s.writee('hello')))

More about Promise.using (a single-resource version is trivial to implement)
http://promise-nuggets.github.io/articles/21-context-managers-transactions.html and about bluebird's (even safer) implementation: http://bluebirdjs.com/docs/api/disposer.html

Not sure how this would look like in the context of streams.

[mafintosh]

I'm sure there are lots of resource management libraries on npm that are better than my above example. I was just showcasing my point. Yes this is just as easy to mess up with callbacks - programming is hard. For me this is mostly about knowing that I messed up some code so I can go back and fix it.

Just yesterday I fixed an issue in https://github.com/feross/bittorrent-dht that was triggering an uncaught exception. Turns out an exception was thrown because of a weird bug in https://github.com/chriso/lru. Both modules are well tested.

Had this exception been handled and simply passed in a callback/promise I wouldn't have seen it.