stream: Fix incorrect error throw when pipe'ing

[kanongil]

Specifically, if an error listener is attached to a stream before it is piped to, and it cleans up efter itself by removing the error listener, the pipe logic will miss this and throw as if it was unhandled.

I'm not sure this is the best way to fix it, but I don't really see any other way with the current EventEmitter interface.

As far as I can tell this error handling error has been there since 0.4.

[kanongil]

I certainly know that, but I'm not sure it can be done much neater without an EventEmitter API update. In <= 0.8.x, listeners('error') can be used instead, but I don't see any way to do it now.

[isaacs]

Moving around the listeners in v0.8 is ALSO too intimate.

I don't get what the problem is. Can you remove the changes to existing tests, and instead just add a new test in that file that shows the problem, and fails with what's in there now?

[kanongil]

Sure, I'll create a separate test. FYI, there are 2 things that need to be changed for the throw to trigger incorrectly.

1. The error listener needs to be attached before the pipe().
2. The error listener needs to remove itself immediate when triggered.

[kanongil]

Btw, I just realized the potential magnitude of problems my approach can generate, and I agree that my initial patch is not suitable to be committed as it is.

[kanongil]

Updates test cases and changed the nature of the fix.

It now uses a shadow '_error' event, that will emit on any 'error', but since it is not directly attached to the 'error' event, it won't prevent it from throwing. In the process it actually cleans up the stream code from EE internals knowledge thereby fixing a domain issue on old style pipe()!

I figure this behavior is somewhat reasonable since 'error' is already special.

[isaacs]

Ok, I grok the problem now.

I'd prefer not to add another "magic" event, even one prefixed by an underscore.

I'll dig into this more this week. I think there is probably a better approach hiding in there somewhere.

[kanongil]

Great, though I dare you to find a solution that doesn't touch EE internals 😉

[kanongil]

I was pondering this, and I have come to the preliminary conclusion that the proper way to solve this is through updating the EE with a new interface.

Essentially you should be able to attach a new class of listener that promises not to change the object state. Something like an 'observer', which will always be called before regular listeners. With this in place, you would only need to change the problematic 'error' listeners to observers.

This mechanism would also be useful in client side code whenever the object state is changed in a listener. If you want to add an additional listener to observe state, you can simply do so without worrying about whether it was attached before or after the state changing listener.

I know the EE API is technically frozen but I believe this change could be implemented in a backwards compatible way.

[Nodejs-Jenkins]

Can one of the admins verify this patch?

[isaacs]

So, having looked through this code and slept on it, I think this patch is actually a good way to go, or at least, the "least bad" way to go. However:

1. It was "broken" for 0.8 and 0.6 just as much as it is in 0.10. There's no urgency to get this changed in 0.10. Let's put it in 0.11, since it changes semantics slightly.
2. I'm afraid that "_error" is too likely to be used by userland code. Maybe we should go with an ES5 style "__error__" event?

Adding a new "observe this event, but don't mess with it" is actually not ideal, since it's only error that we'd ever want this for.

@bnoordhuis or @indutny, can you comment?

[bnoordhuis]

I like __error__ but that's because I'm a closet pythonista.

[kanongil]

@isaacs I agree regarding the label change. I just put the first thing that came to mind, figuring that someone would have an opinion one way or the other.

Regarding the "observe" handler, I still think it could potentially be useful, but I can't seem to think of any killer use-cases.

Finally, I don't see why this can't be applied as a straight up bugfix. Is it the nature of the bug or the fix you are worried about?

[isaacs]

So, the degree of change that is tolerated in a stable branch depends somewhat on the severity of the bug, the subtlety of the change, and the stability index of the code being touched.

In this case, we're talking about a bug that is pretty edge-case, so not all that severe. It only occurs if you have a once handler on an error, and assign it at a specific time, relative to pipe() being called, and we've had it since at least 0.4.

It's very subtle, since its possible that someone might have code that's functioning as they intend with this bug, and we'll change it in an unexpected way by "fixing it".

Lastly, it touches an extremely high-stability section of the code, so unless it's something glaring and severe, I'd tend to push to do it in master instead, just to err on the side of caution.

Regarding the "observe" handler, I still think it could potentially be useful, but I can't seem to think of any killer use-cases.

Sounds like a userland module to me :)

[kanongil]

Great, thanks for the answer. I'd say that the bug in kinda severe since the program can exit for errors you thought were handled. Anyway I agree on the other points, and I'm fine with simply pushing it to master.

Given this, you should probably create a separate fix for the old pipe() throw so that it respects domains, like the new one.

[kanongil]

Just realized that this is closely related to #4155.

Also, do you want me to update the patch with __error__?

[isaacs]

Yeah, this patch would fix #4155. It's the same error. Feel free to put Fix #4155 in the commit message.

Yeah, please update patch with __error__ as the event name. It'd also be nice to have a test that emits "error" and verifies that "__error__" got emitted as well.

It seems like it would also be worthwhile to have something that is given first-class treatment and only emits when an error is not handled. That should probably be a separate pull req, and maybe wait until we have a specific need for it.

[kanongil]

OK, I have rebased and fixed the patch, now split it into 3 sub patches.

1. I have created entirely new test cases for EE 'error' throwing behavior, as I could not find any existing ones.
2. Separate patch for new '__error__' emit with tests.
3. The actual fix using the new '__error__' handler.

[kanongil]

Any comments?

[isaacs]

Fixed without the addition of a new special event in 23d92ec. Thanks!

[kanongil]

The first patch is still a relevant generic test, though it might have been covered since I wrote it? Also, my solution doesn't silently ignore uncatched errors.