Skip to content

require same transports unless upgrading #177

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

Closed
wants to merge 1 commit into from
Closed

require same transports unless upgrading #177

wants to merge 1 commit into from

Conversation

aeosynth
Copy link

@aeosynth
Copy link
Author

@guille ping? i've been using this in production, and have stopped receiving complaints about connection errors

@rauchg
Copy link
Contributor

rauchg commented Jun 23, 2013

I think we need more comments in the code to explain the rationale. We also need to document the new parameter in verify

@rauchg
Copy link
Contributor

rauchg commented Jun 23, 2013

This absolutely needs tests as well to avoid regressions

@deoxxa
Copy link

deoxxa commented Jul 9, 2013

Any action on this? I've just found that the same thing happens on a particular wireless operator in Australia, owing to their use of a transparent proxy implemented with squid. If there are no current plans to write tests and get this merged, I'd be willing to wade around in whatever the testing bits are here and figure it out, but it would probably take me a lot longer than it'd take someone who's already familiar with engine.io internals.

@aeosynth
Copy link
Author

aeosynth commented Jul 9, 2013

Go for it.
On Jul 8, 2013 7:48 PM, "Conrad Pankoff" [email protected] wrote:

Any action on this? I've just found that the same thing happens on a
particular wireless operator in Australia, owing to their use of a
transparent proxy implemented with squid. If there are no current plans to
write tests and get this merged, I'd be willing to wade around in whatever
the testing bits are here and figure it out, but it would probably take me
a lot longer than it'd take someone who's already familiar with engine.iointernals.


Reply to this email directly or view it on GitHubhttps://github.com//pull/177#issuecomment-20650313
.

@kapouer
Copy link
Contributor

kapouer commented Nov 25, 2013

Please comment this test: https://gist.github.com/kapouer/7634923

@rauchg
Copy link
Contributor

rauchg commented Nov 25, 2013

Thanks a lot @kapouer you the man

@defunctzombie
Copy link
Contributor

Is this PR still relevant?

@rauchg
Copy link
Contributor

rauchg commented Feb 2, 2014

Very much so. Upgrading breaks polling on Squid and others.

@rauchg
Copy link
Contributor

rauchg commented Feb 2, 2014

Because the websocket request gets passed as a regular request which then makes the socket error. It should be ignored instead. The carnegie guys are working on this one.

nlagrow pushed a commit to nlagrow/engine.io that referenced this pull request Feb 8, 2014
Some proxies - such as nginx and squid - don't support websocket. The client attempting to upgrade its transport caused the engine.io connection to break (see socketioGH-177). THe server now checks within handleRequest to see if the transport has changed without an upgrade request - if this is the case, the upgrade request is denied (defaults to polling) and the connection is maintained
@rauchg rauchg closed this Feb 8, 2014
@aeosynth aeosynth deleted the squid branch February 9, 2014 03:29
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.

error with squid proxy
5 participants