juliangruber / multipipe

A better `Stream.pipe` that creates duplex streams and lets you handle errors in one place.
294 stars 23 forks source link

Why duplexer2 calling with bubbleErrors equal false? #47

Closed serp-dev closed 5 years ago

serp-dev commented 5 years ago

Hey.

Error handling does not work how it written in the documentation. If the first stream writable and last stream readable, they wrap in duplexer2 with "bubbleErrors" option equal false (by default).

Multipipe will not emit error, in this example:

var multipipe = require('multipipe'); // 1.0.2 version
var JSONStream = require('JSONStream');
var through2 = require('through2');
var https = require('https');

writableStream = through2(function(data, enc, callback) {
    callback(null, data);
});

https.get('https://github.com/juliangruber/multipipe/blob/master/Readme.md', function(res) {
    res.pipe(writableStream);
});

var readableStream = JSONStream.parse('*');

readableStream.on('error', function(err) {
    // it works
    console.log('Readble stream ERROR: ', err);
});

var testStream = multipipe(
    writableStream,
    readableStream
).on('error', function(err) {
    // it not works
    console.log('Multipipe ERROR: ', err)
});

Why is this done?

juliangruber commented 5 years ago

In this line we should make sure that any stream that isn't the returned stream (which in this case none is) will forward its errors to the returned stream:

https://github.com/juliangruber/multipipe/blob/50ca690e9edd5889dae2105b87618176aa3cc42b/index.js#L62

EDIT: we're specifically disabling bubbleErrors since we implement that functionality ourselves. You'll see that enabling bubbleErrors won't fix your example.

This seems to be an issue with JSONStream, see https://github.com/dominictarr/JSONStream/issues/136

I modified your script to not use JSONStream but a regular stream that emits an error event, and that is properly caught by multipipe:

var {PassThrough} = require('stream')
var multipipe = require('multipipe'); // 1.0.2 version
var through2 = require('through2');
var https = require('https');

writableStream = through2(function(data, enc, callback) {
  callback(null, data);
});

https.get('https://github.com/juliangruber/multipipe/blob/master/Readme.md', function(res) {
  res.pipe(writableStream);
});

var readableStream = new PassThrough()
setImmediate(() => readableStream.emit('error', new Error('bloob')))

multipipe(
  writableStream,
  readableStream
).on('error', function(err) {
  // it not works
  console.log('Multipipe ERROR: ', err)
});
anton-makarov commented 5 years ago

JSONStream throws error ok, the problem is in duplexer2. If read stream doesn't implement read method, duplexer wraps it in Readable from readable-stream lib:

  if (typeof readable.read !== "function") {
    readable = (new stream.Readable(options)).wrap(readable);
  }

and in wrap function listeners on all event are applied:

  // proxy certain important events.
  for (var n = 0; n < kProxyEvents.length; n++) {
    stream.on(kProxyEvents[n], this.emit.bind(this, kProxyEvents[n]));
  }

But, if bubbleErrors hasn't been passed, then no error listeners will be applied to wrapped stream and in case of error in read stream, error will be thrown right into process, crashing everything. Listeners in multipipe don't help because they are applied after listeners in wrap function.

Right now don't know how to fix it, other then to pass bubbleErrors in options, but it doesn't feel as the right way :(

okv commented 5 years ago

I've encountered same issue. If readable stream doesn't implement read method (which is true for old stream api) then error on that stream will not be caught by multipipe (when bubbleErrors: false). E.g. all streams produced by through don't have read method - it's a lot of streams) Seems like bubbleErrors: true fixes the problem. @juliangruber could you clarify, why in this case you don't want to bubbleErrors: true by default? Could it break something?

juliangruber commented 5 years ago

I'll take another look!

okv commented 5 years ago

thanks, will be waiting for your news

serp-dev commented 5 years ago

Thanks publish new version please)

okv commented 5 years ago

awesome, thanks