Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

stream: Throw on 'error' if listeners removed #6075

Merged
merged 1 commit into from
Aug 19, 2013
Merged

Conversation

isaacs
Copy link

@isaacs isaacs commented Aug 19, 2013

In this situation:

writable.on('error', handler);
readable.pipe(writable);
writable.removeListener('error', handler);
writable.emit('error', new Error('boom'));

there is actually no error handler, but it doesn't throw, because of the
fix for stream.once('error', handler), in 23d92ec.

Note that simply reverting that change is not valid either, because
otherwise this will emit twice, being handled the first time, and then
throwing the second:

writable.once('error', handler);
readable.pipe(writable);
writable.emit('error', new Error('boom'));

Fix this with a horrible hack to make the stream pipe onerror handler
added before any other userland handlers, so that our handler is not
affected by adding or removing any userland handlers.

Closes #6007.

@isaacs
Copy link
Author

isaacs commented Aug 19, 2013

Oh, whoops, this should be against v0.10, not master. If someone LGTM's it, I can merge.

@indutny
Copy link
Member

indutny commented Aug 19, 2013

LGTM.

In this situation:

    writable.on('error', handler);
    readable.pipe(writable);
    writable.removeListener('error', handler);
    writable.emit('error', new Error('boom'));

there is actually no error handler, but it doesn't throw, because of the
fix for stream.once('error', handler), in 23d92ec.

Note that simply reverting that change is not valid either, because
otherwise this will emit twice, being handled the first time, and then
throwing the second:

    writable.once('error', handler);
    readable.pipe(writable);
    writable.emit('error', new Error('boom'));

Fix this with a horrible hack to make the stream pipe onerror handler
added before any other userland handlers, so that our handler is not
affected by adding or removing any userland handlers.

Closes nodejs#6007.
@kanongil
Copy link

Great! We have now come full circle on this issue, and have arrived at my originally proposed fix which you advocated against at the time.

@isaacs isaacs merged commit 5458079 into nodejs:master Aug 19, 2013
@kanongil kanongil mentioned this pull request Jun 10, 2015
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants