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

caronte: ws-incoming stream seems to ignore head #473

Closed
glasser opened this issue Sep 16, 2013 · 2 comments
Closed

caronte: ws-incoming stream seems to ignore head #473

glasser opened this issue Sep 16, 2013 · 2 comments

Comments

@glasser
Copy link
Contributor

glasser commented Sep 16, 2013

I'm skimming the source of the caronte branch before rewriting my project (Meteor) to use its new API. (I've definitely been looking forward to this for a while since this is the main thing keeping Meteor stuck on Node 0.8!) caronte is much easier to understand than the old code!

I notice that the stream pass in ws-incoming ignores the head argument. ie, if there is any socket body data in the initial buffer sent to the http parser where the upgrade header it is detected, this head is the only access you get to those initial bytes, and they won't show up in a later stream from the socket. You properly unshift the equivalent argument from the connection between the proxy and the proxy target, but the head (the data sent from the client) seems to be ignored.

As I said I have not actually tested caronte in practice yet; this issue is based just on reading the code. But the improper handling of this argument is related to the reason that the current http-proxy release is broken on 0.10 so I suspect that this is a bug.

@cronopio
Copy link
Contributor

Hi @glasser thanks for take some time in reading the caronte branch.

I just fixed the test suite in the caronte-tests according to the new changes, so, if you can propose a test case where we can reflect the behaivor that you said, would be very useful.

More info on #476

Thanks!!

yawnt added a commit that referenced this issue Sep 17, 2013
@yawnt
Copy link
Contributor

yawnt commented Sep 17, 2013

sounds about right.. fixed.. thanks
also what @cronopio said.. it would be real useful :)

@yawnt yawnt closed this as completed Sep 17, 2013
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

No branches or pull requests

3 participants