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

Broken on node v5.6.0 #26

Closed
mcollina opened this issue Feb 10, 2016 · 1 comment
Closed

Broken on node v5.6.0 #26

mcollina opened this issue Feb 10, 2016 · 1 comment

Comments

@mcollina
Copy link
Collaborator

bl tests are failing on node v5.6.0 because of nodejs/node#4951. What is failing is appending multiple buffer list within themselves: https://github.com/rvagg/bl/blob/master/test/test.js#L79-L94.

I am not sure we were relying on some broken functionality of core (not checking if arguments were buffers). In fact, the only thing of Buffer is relying on is a copy method.

Not sure what is the best fix for this, either on node or here.

Also reported here: moscajs/mosca#412.

cc @rvagg

@mcollina
Copy link
Collaborator Author

I think the best fix is to "recognize" if the passed in instance is a 'bl' instance, and append all the single buffers. This would be "introspecting" the passed instance, but I think that is better than allocating a big buffer for the sole purpose of appending it to another list.

The original code also might create issues when appending to the nested bl, so we should probably fix it here.

mcollina added a commit that referenced this issue Feb 10, 2016
This provides support for node v5.6.0.

Fixes #26.
mcollina added a commit that referenced this issue Feb 11, 2016
This provides support for node v5.6.0.

Fixes #26.
@rvagg rvagg closed this as completed in 79feb5b Feb 11, 2016
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 a pull request may close this issue.

1 participant