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

Stream's api methods must never throw - handling errors in DecodeStream, EncodeStream #81

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

royaltm
Copy link

@royaltm royaltm commented Jan 12, 2018

Hello there!

This is just a quick fix for #75.
Nodejs stream api is callback based and doesn't handle errors thrown inside api methods.
I've just wrapped in try/catch block implementation of Transform stream in DecodeStream and EncodeStream to handle errors according to stream specs. It's rather mandatory to be able to handle errors when working with streams. In current version of msgpack-lite node process dies due to unhandled error when such error occures: e.g. DecodeStream fed with garbage data or EncodeStream with circular references.

Test cases included.

Copy link

@DarrenCook DarrenCook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me.

The before code uses if(callback)..., but the after code does not. Should that be added, just in case? Or is callback guaranteed to always be a callable object when working with streams?

@royaltm
Copy link
Author

royaltm commented Mar 27, 2018

AFAIK the callback is guaranteed and mandatory in node's stream api: https://nodejs.org/dist/latest-v8.x/docs/api/stream.html#stream_transform_transform_chunk_encoding_callback
IDK why it was like that.

@kawanet
Copy link
Owner

kawanet commented Mar 27, 2018

Thanks for the patch and the approval comment.
The patch looks good and minimal. I like the style. I will be merging it, asap.
I'm sorry for the late replies. I was too busy for those months.

@qqtrend
Copy link

qqtrend commented Jun 11, 2018

@kawanet Is it possible to merge this now? The Travis failure appears to be unrelated. I.e. all pull requests are failing at the same point (something to do with IE10?)

@robertsLando
Copy link

@kawanet Ping?

@kawanet
Copy link
Owner

kawanet commented Sep 29, 2020

pong

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.

None yet

5 participants