Skip to content
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

Why duplexer2 calling with bubbleErrors equal false? #47

Closed
serp-dev opened this issue Nov 23, 2018 · 7 comments
Closed

Why duplexer2 calling with bubbleErrors equal false? #47

serp-dev opened this issue Nov 23, 2018 · 7 comments

Comments

@serp-dev
Copy link

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
Copy link
Owner

juliangruber commented Nov 23, 2018

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:

if (stream !== ret) stream.on('error', err => ret.emit('error', err))

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 dominictarr/JSONStream#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
Copy link

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
Copy link

okv commented Dec 7, 2018

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 juliangruber reopened this Dec 7, 2018
@juliangruber
Copy link
Owner

I'll take another look!

@okv
Copy link

okv commented Dec 7, 2018

thanks, will be waiting for your news

@serp-dev
Copy link
Author

serp-dev commented Dec 7, 2018

Thanks
publish new version please)

@okv
Copy link

okv commented Dec 7, 2018

awesome, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants