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

Handle chunked multibyte characters #25

Merged
merged 1 commit into from
Dec 12, 2016

Conversation

dmarkow
Copy link
Contributor

@dmarkow dmarkow commented Aug 18, 2016

In some large messages (in my case, roughly 8MB strings with very large JSON objects) I was running into JSON.parse failures on the server side.

Before this fix, if the split between different chunks in a message occurred in the middle of a two-byte string, the first byte would still be parsed by data.toString() and converted into a � character. Then the second byte in the next chunk would also be converted into a � character. This would then throw the length of the string off increasing it by 1, and since this library uses the content length prefix at the beginning of the string, it would effectively ignore the last character, which is often a } or ]. So JSON.parse would fail because of the missing closing bracket.

Node's built-in StringDecoder module is made specifically to ensure decoded strings don't contain incomplete multibyte characters. When it tries to decode a buffer that has an incomplete character, it'll set that last byte or two aside and hold on to them until the next time it's called.

I believe this will address the concerns in #11.

SamuelBolduc added a commit to SamuelBolduc/node-json-socket that referenced this pull request Dec 12, 2016
@SamuelBolduc
Copy link
Collaborator

Thank you for your PR. I personally encountered problems in the past that were most likely related to this issue.

For some reason Travis has a hard time running the tests on the pull-request, but I validated the fix and all the tests, including the new one submitted with the commit, pass without any issue. I also tested the code in real-world situation and it works flawlessly.

@SamuelBolduc SamuelBolduc merged commit b320b99 into sebastianseilund:master Dec 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants