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 osc.readString not fail on very large strings #92

Merged
merged 8 commits into from
May 15, 2019

Conversation

okofish
Copy link
Contributor

@okofish okofish commented Jul 15, 2017

Previously, osc.readString used String.charCodeAt.apply with a TypedArray consisting of the entire OSC argument. This would fail with a RangeError on very large (>~60k-100k bytes) string arguments, as it would hit the limit on the number of arguments to a function.

This PR checks for the existence of Buffer (Node.js/Electron) or TextDecoder (modern browsers) and uses them to decode the TypedArray if available. If not, it will use charCodeAt as before, but it applies it to the array in chunks so as not to hit the argument limit. When using Buffer or TextDecoder, this has the added benefit of properly decoding multi-byte Unicode characters. Killing two bugs with one patch!

I added a test for large string behavior but not for multi-byte character decoding, as this is not necessarily universal behavior. I always have issues with Electron and node-serialport, so I would appreciate if someone else could run the Electron tests.

okofish added 5 commits July 14, 2017 22:47
…lable

Otherwise, it will use charCodeAt as before, but it applies it to the array in chunks so as not to hit the argument limit.
okofish added a commit to okofish/qlab-html that referenced this pull request Jul 15, 2017
@colinbdclark
Copy link
Owner

Thanks for the PR, @okofish. I'll take a look as soon as I have some spare time!

@okofish
Copy link
Contributor Author

okofish commented May 14, 2018

Hey, did you get a chance to look at this yet?

colinbdclark added a commit that referenced this pull request Oct 31, 2018
* okofish/master:
  Updating dist files
  Updating dist files
  Adding tests
  Changing checks for Buffer/TextDecoder
  Fixing style
  Changing osc.readString to use Buffer.toString or TextDecoder if available
@colinbdclark
Copy link
Owner

Hi @okofish, I'm so sorry it took me so incredibly long to look more closely at this pull request. Your PR looks good; I've done some minor refactoring to your code. Does this seem reasonable?

Do you think we should be worried about the asymmetry of supporting UTF-8 characters when reading OSC messages but not while writing them? Should we be using the equivalent TextEncoder approach in osc.writeString?

I'm tempted to go ahead and merge this and not worry too much about it for now, but wanted to gather your thoughts first.

Again, apologies for the delay and thank you very much for submitted this PR, I appreciate it.

@okofish
Copy link
Contributor Author

okofish commented Nov 2, 2018

Refactors look fine. I would tend to think that TextEncoder in osc.writeString would be a good idea. Incidentally, I haven't tested this but Node.js v8.3 (released after I made this PR) seems to have TextEncoder and TextDecoder available under the util module, and in the just-released v11 they were made globals. It would make sense to use TextEncoder/TextDecoder by default if they're available as globals or from util, then Buffer, and finally fromCharCode. But feel free to merge as-is if you don't want to make this change :)

colinbdclark added a commit that referenced this pull request May 15, 2019
@colinbdclark colinbdclark merged commit 1cf77e2 into colinbdclark:master May 15, 2019
colinbdclark added a commit that referenced this pull request May 15, 2019
* gh-92:
  Updates pre-built files.
  Removes use of Buffer constructor now that Node 10 is LTS.
  Uses TextDecoder in Node.js when available.
  gh-92: Refactors for modularity and simpler environment detection.
  Updating dist files
  Updating dist files
  Adding tests
  Changing checks for Buffer/TextDecoder
  Fixing style
  Changing osc.readString to use Buffer.toString or TextDecoder if available
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