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

Fix bug with wrtc not handling Buffers correctly #61

Closed
wants to merge 5 commits into from

Conversation

paulkernfeld
Copy link

In this pull request, I also added back in the wrtc: wrtc arguments in the tests, which I see that you recently removed. If you want those to stay out, I can remove that commit from this PR.

Paul Kernfeld added 3 commits January 28, 2016 12:50
Add a line to test/common that exports a WebRTC implementation
Always convert a Buffer to UInt8Array, even if it's a typed array
...to make sure that we're not accidentally null-terminating our messages
@paulkernfeld
Copy link
Author

I tested this locally against Chrome 47 and Firefox 43 on OSX, and it passed.

@feross
Copy link
Owner

feross commented Jan 28, 2016

The commits to add zeroes the binary test and make the tests easier to run on node are fine.

But the core of the PR, I'm not sure about. This will cause an extra copy on every send, even in non-wrtc situations, which is likely to have performance implications.

@paulkernfeld
Copy link
Author

Oh, I see. Yeah, that seems like it could be bad. Would you be opposed to code that special-cases wrtc and only converts from a Buffer in that case? Or, is there some way to convert data from a Buffer without doing a copy?

Another solution might be a simple polyfill library that tries to abstract away some of this stuff.

@feross
Copy link
Owner

feross commented Jan 28, 2016

I think a bit of code that special cases wrtc is the best way to go.

There's no real way to solve this for wrtc without a copy it seems, since the problem is that it doesn't like the subclassed Uint8Array.

Paul Kernfeld added 2 commits January 28, 2016 16:06
@paulkernfeld
Copy link
Author

All right, here's a commit that special-cases wrtc. I think there are probably a million ways to do this, so I'm happy to change this if there's a better way.

@feross
Copy link
Owner

feross commented Jan 28, 2016

Thanks for updating your PR. I think I found a cleaner way to do this for now. Let's just use the existence of the wrtc option to detect wrtc. It's currently the only implementation in node anyway.

Once another viable alternative exists in node, or the issue is fixed in wrtc, we can revisit this.

Published as 5.11.9.

@feross feross closed this Jan 28, 2016
@paulkernfeld
Copy link
Author

Awesome!

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

2 participants