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

Update to new faye-websocket/websocket-driver with new http parser #160

Closed
sdarnell opened this Issue Aug 1, 2017 · 3 comments

Comments

Projects
None yet
3 participants
@sdarnell

sdarnell commented Aug 1, 2017

As per meteor/meteor#8648 I discovered a problem with the websocket-driver package used by faye-websocket. It was causing an assert in debug builds of node, and there is a chance that it also results in some form of memory corruption on refresh - though it is hard to say for certain.

The websocket-driver code has been updated to use a new http parser and avoid using node internals. So this feature request is a placeholder to make sure it gets updated when the faye-websocket/websocket-driver gets released.

See faye/websocket-driver-node#21

I have manually dropped in replacements and it seems to work in meteor, but I'm unable to reproduce the crashes that others are seeing so I can't be certain whether it fixes meteor/meteor#8648

@sdarnell

This comment has been minimized.

sdarnell commented Aug 1, 2017

For reference, the current (old) versions are:
faye-websocket: 0.11.1 https://www.npmjs.com/package/faye-websocket
websocket-driver: 0.6.5 https://www.npmjs.com/package/websocket-driver

@hwillson

This comment has been minimized.

Member

hwillson commented Aug 1, 2017

Thanks @sdarnell - I've marked this as pull-requests-encouraged. If anyone is interested in working on this, please wait until after the changes from faye/websocket-driver-node#21 have been published and made available via a new faye-websocket release.

benjamn added a commit to meteor/meteor that referenced this issue Aug 7, 2017

@abernix

This comment has been minimized.

Member

abernix commented Sep 4, 2017

Closing this as the PR has been merged. This will land in Meteor 1.5.2 and beyond. Thanks very much, @sdarnell!

@abernix abernix closed this Sep 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment