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

Make the Readable constructor handle 'close' events without 'end'. Fixes #478. #479

Merged
merged 5 commits into from
Apr 19, 2016

Conversation

vqvu
Copy link
Collaborator

@vqvu vqvu commented Apr 14, 2016

It's possible for a stream to emit close but not end (e.g., when trying to read a non-existant file). We need to end the wrapper stream in that case. Previously, the stream would simply never end.

Ref #478.

vqvu added 3 commits April 14, 2016 03:36
This adds a bunch of test.expect in the appropriate places.
It's possible for a stream to emit 'close' but not 'end' (e.g., when
trying to read a non-existant file). We need to end the wrapper stream
in that case.
@vqvu
Copy link
Collaborator Author

vqvu commented Apr 15, 2016

Ok, so it's also possible to only get an error from the Readable. This is what actually happens when reading a non-existent file. The close case is when the user explicitly calls close() or something.

The only way to fix this is to emit nil on our end when we encounter an error. This stack overflow answer seems to suggest that this is the correct behavior.

end

When an error event is fired, the end event will not be fired(explicitly). The emitting of an error event will end the stream.

The only thing that makes me a bit nervous is that the Node docs do not mention anything about the semantics of the error event, and the code will happily allow implementations to call error multiple times. The only exception being the fact that pipe will automatically unpipe all destinations when any destination emits error.

So even though this semantics makes perfect sense to me**, I feel compelled to ask if anyone knows of a readable stream that can emit multiple errors.

** Ironic, since Highland's own error semantics is the opposite.

@vqvu vqvu force-pushed the readable-handle-close branch from 0d1ac07 to 2551b45 Compare April 15, 2016 02:20
@vqvu
Copy link
Collaborator Author

vqvu commented Apr 19, 2016

Merging this since nobody seems to object.

@vqvu vqvu merged commit c8177fb into caolan:master Apr 19, 2016
@vqvu vqvu deleted the readable-handle-close branch April 19, 2016 21:17
@jgrund
Copy link
Collaborator

jgrund commented Sep 2, 2016

Just seeing this now, but I actually have a case of a stream that will emit error but not close/end. I'll open a separate issue.

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

Successfully merging this pull request may close these issues.

2 participants