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

Fixes issue commented on in #377 #501

Merged
merged 3 commits into from
Aug 31, 2011
Merged

Fixes issue commented on in #377 #501

merged 3 commits into from
Aug 31, 2011

Conversation

einaros
Copy link
Contributor

@einaros einaros commented Aug 31, 2011

See the following issue comment: #377 (comment)

Handshakes weren't correctly passing a content-type header, making some browsers complain about errors parsing xml, although the data is really text/plain.

The pull req also contains two other minor style fixes as a bonus :P

@3rd-Eden
Copy link
Contributor

LTGM

rauchg added a commit that referenced this pull request Aug 31, 2011
Fixes issue commented on in #377
@rauchg rauchg merged commit 7c196f5 into socketio:master Aug 31, 2011
@@ -775,6 +775,7 @@ Manager.prototype.handleHandshake = function (data, req, res) {
res.writeHead(200, { 'Content-Type': 'application/javascript' });
res.end('io.j[' + data.query.jsonp + '](new Error("' + message + '"));');
} else {
res.writeHead(200, { 'Content-Type': 'text/plain' });
res.writeHead(status);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Two writeHeads ?
  • Why 200 in writeErr? (the other 200 is for jsonp, which we have to do since it has no error handling)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The jsonp response originally had 200, so I kept it for the plaintext
response as well. This isn't as much an http error as a handshake
interpreted by script error, no?

On Aug 31, 2011 5:52 PM, "guille" <
reply@reply.github.com>
wrote:

@@ -775,6 +775,7 @@ Manager.prototype.handleHandshake = function (data,
req, res) {
res.writeHead(200, { 'Content-Type': 'application/javascript' });
res.end('io.j[' + data.query.jsonp + '](new Error%28"' + message +);
} else {

  • res.writeHead(200, { 'Content-Type': 'text/plain' });
    res.writeHead(status);

Two writeHeads ?
Why 200 in writeErr?

Reply to this email directly or view it on GitHub:
https://github.com/LearnBoost/socket.io/pull/501/files#r108484

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why the jsonp one passes an Error object so that we can do a quick instanceof check to know if the response was successful or not.
For the regular handshake, we of course just inspect the status code. That's why the original has res.writeHead(status), now duplicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the duplicate writehead is bogus. And you're of course right about the
status code. I'll correct both.

On Aug 31, 2011 7:11 PM, "guille" <
reply@reply.github.com>
wrote:

@@ -775,6 +775,7 @@ Manager.prototype.handleHandshake = function (data,
req, res) {
res.w...
That's why the jsonp one passes an Error object so that we can do a quick
instanceof check to know if the response was successful or not.
For the regular handshake, we of course just inspect the status code. That's
why the original has res.writeHead(status), now duplicated.

Reply to this email directly or view it on GitHub:
https://github.com/LearnBoost/socket.io/pull/501/files#r108609

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks <3

@pyrostrex
Copy link

@guille, @einaros, after some tests with socket.io after this update. It turns out that I must have einaros socket.io-client fork to get it work with FF6. Care to check it for me?. Thanks in advance!

I tried these :

Socket.IO (git cloned) and Einaros Socket.IO-client (git cloned) ---- works
Socket.IO (git cloned) and Socket.IO-client (git cloned) ---- fails
Einaros Socket.IO (git cloned) and Einaros Socket.IO-client (git cloned) ---- works

@einaros
Copy link
Contributor Author

einaros commented Sep 1, 2011

@pyrostrex, Use FireBug's network tab and verify that FF6 actually tries to use WebSocket as transport with the LearnBoost repo versions, and not actually falls back to FlashSocket. I've made a few updates to my socket.io-client, which allows even WebSocket to fall back to FlashSocket, so this may be what's happening with my builds.

@pyrostrex
Copy link

@einaros, I didn't enable flashsocket. I even use io.set('transports', ['websocket']). I installed firebug and see the network tab just like you did in your comment. and I can't see any "upgrade to websocket" request in firebug. Here's what i did to setup the socket.io:

cd node_modules
git clone https://github.com/LearnBoost/socket.io.git
cd socket.io
npm update
cd node_modules
rm -rf socket.io-client
git clone https://github.com/LearnBoost/socket.io-client.git
cd socket.io-client
npm update

But when I do the same, but instead of cloned from socket.io-client, I cloned from your repo. Then I refresh my browser. Then the firebug shows the "upgrade to websocket" request and everything else works.

@pyrostrex
Copy link

@einaros, It turns out I forgot to build. Now it's ok. :)

@einaros
Copy link
Contributor Author

einaros commented Sep 1, 2011

@pyrostrex, Ah -- well I'm glad you figured it out :)

To just straighten this out once and for all, though: you had to build the cloned LearnBoost/socket.io-client before you could get it working?

@pyrostrex
Copy link

@einaros, Yep! on my step. add another step which is "node builder.js". haha. Now trying to find a way to get socket.io websocket to work with latest windows chrome.

@einaros
Copy link
Contributor Author

einaros commented Sep 1, 2011

@pyrostrex, What you cloned from the socket.io repo should already have been
built, so that's unfortunate.

Regarding WebSockets with the latest Chrome - are you having trouble getting
that going? It should work out of the box. If it doesn't, which specific
version of Chrome are you running?

On Thu, Sep 1, 2011 at 3:24 PM, pyrostrex <
reply@reply.github.com>wrote:

@einaros, Yep! on my step. add another step which is "node builder.js".
haha. Now trying to find a way to get socket.io websocket to work with
latest windows chrome.

Reply to this email directly or view it on GitHub:
#501 (comment)

@pyrostrex
Copy link

@einaros, I'm using 13.0.782.218 m of chrome, like I said Windows only. In Mac, it doesn't have this problem. only in Windows. This time, it is much more weirder, it raises the "connection" event on the server but on the client, it still shows "pending" on the network tab.

@einaros
Copy link
Contributor Author

einaros commented Sep 1, 2011

@pyrostrex, Ok, 13.0.782.218 uses the old hixie-76 WebSocket standard, so it
isn't as such touched by the changes as of late.

Did you happen to see if my forks work with that version of Chrome?

On Thu, Sep 1, 2011 at 3:41 PM, pyrostrex <
reply@reply.github.com>wrote:

@einaros, I'm using 13.0.782.218 m of chrome, like I said Windows only. In
Mac, it doesn't have this problem. only in Windows. This time, it is much
more weirder, it raises the "connection" event on the server but on the
client, it still shows "pending" on the network tab.

Reply to this email directly or view it on GitHub:
#501 (comment)

@pyrostrex
Copy link

@einaros, It still happens with your forks. Here is my output:

Server output:

debug - served static /socket.io.js
debug - client authorized
info  - handshake authorized 10075011381301021817
debug - setting request GET /socket.io/1/websocket/10075011381301021817
debug - set heartbeat interval for client 10075011381301021817
debug - client authorized for

Browser Network Output :

URL    Method    Status    Type
/socket.io/1/?t=1314885489724    GET     200    text/plain
10075011381301021817/socket.io/1/websocket    GET    (Pending)    undefined

Additional Information :

Using Namespace.
Your Forks. Server and Client

@einaros
Copy link
Contributor Author

einaros commented Sep 1, 2011

@pyrostrex, Ok - does this work with the WebSocket transport on other
browsers than the Chrome13 you've tested with? If not, is your site behind a
proxy (such as nginx)?

On Thu, Sep 1, 2011 at 4:02 PM, pyrostrex <
reply@reply.github.com>wrote:

@einaros, It still happens with your forks. Here is my output:

Server output:

debug - served static /socket.io.js
debug - client authorized
info - handshake authorized 10075011381301021817
debug - setting request GET /socket.io/1/websocket/10075011381301021817
debug - set heartbeat interval for client 10075011381301021817
debug - client authorized for

Browser Network Output :

URL Method Status Type
/socket.io/1/?t=1314885489724 GET 200 text/plain
10075011381301021817/socket.io/1/websocket GET (Pending)
undefined

Additional Information :

Using Namespace.

Reply to this email directly or view it on GitHub:
#501 (comment)

@pyrostrex
Copy link

@einaros, I'm using pure nodejs.

Wow. It seems to also not working with firefox 6 Windows. For Mac, every browser works (chrome, firefox, safari). For Windows, everything doesn't work. I think it have something to do with the packet parsing thingy?

@einaros
Copy link
Contributor Author

einaros commented Sep 1, 2011

@pyrostrex, It's unlikely that it's because of the packet parsing, seeing as
this only affects hybi07->

See if http://draw.2x.io works from your Windows box. I've got a (fairly
recent) clone of my forks running there.

On Thu, Sep 1, 2011 at 4:35 PM, pyrostrex <
reply@reply.github.com>wrote:

@einaros, I'm using pure nodejs.

Wow. It seems to also not working with firefox 6 Windows. For Mac, every
browser works (chrome, firefox, safari). For Windows, everything doesn't
work. I think it have something to do with the packet parsing thingy?

Reply to this email directly or view it on GitHub:
#501 (comment)

@pyrostrex
Copy link

@einaros, I tried your website. It works but this is because of the websocket to flashsocket switching method. My windows is Windows Thin PC and I tried using chrome.

@einaros
Copy link
Contributor Author

einaros commented Sep 1, 2011

@pyrostrex, Chrome switched to FlashSocket for your connection to my site?
This is strange indeed - I'm running the exact same Chrome version you are,
and I get a WebSocket connection to it.

On Thu, Sep 1, 2011 at 4:52 PM, pyrostrex <
reply@reply.github.com>wrote:

@einaros, I tried your website. It works but this is because of the
websocket to flashsocket switching method. My windows is Windows Thin PC.

Reply to this email directly or view it on GitHub:
#501 (comment)

@pyrostrex
Copy link

@einaros, Yeah, maybe because my internet connection is extremely slow? 10kbps? I tried using Firefox 6.0.1, It never stop loading. haha.

@einaros
Copy link
Contributor Author

einaros commented Sep 1, 2011

@pyrostrex, Which addons are you using for Chrome and FireFox? And which
antivirus are you currently running? Does your antivirus come with any
additional firewall?

On Thu, Sep 1, 2011 at 5:01 PM, pyrostrex <
reply@reply.github.com>wrote:

Yeah, maybe because my internet connection is extremely slow? 10kbps? I
tried using Firefox 6.0.1, It never stop loading. haha.

Reply to this email directly or view it on GitHub:
#501 (comment)

@pyrostrex
Copy link

@einaros, Lol, I'm using fresh installed windows. Inside a VirtualBox. So no other stuff installed other than browsers.

Edit : Chrome and Firefox has firebug installed inside.

@einaros
Copy link
Contributor Author

einaros commented Sep 1, 2011

@pyrostrex, Hm, strange indeed. Your internet connection shouldn't be a case
at all for your local testing, and from what I gather you're really seeing
the same problems there, as you are against my site - save for the fact that
you've disabled flashsocket for your app. At this point you really should do
some digging deep with Fiddler or even WireShark, to see what is going on
with the WebSocket connections.

If you're able to do a clean WireShark dump of the WebSocket traffic from
your browser to your app, I'd be happy to look at that as well.

On Thu, Sep 1, 2011 at 5:05 PM, pyrostrex <
reply@reply.github.com>wrote:

@einaros, Lol, I'm using fresh installed windows. Inside a VirtualBox. So
no other stuff installed other than browsers.

Reply to this email directly or view it on GitHub:
#501 (comment)

@pyrostrex
Copy link

@einaros, Never know how to use WireShark. But I will try.

vanquisher0411 added a commit to vanquisher0411/socketio_project that referenced this pull request Feb 29, 2024
…syntax error message in the Web Console. Basically an addition to socketio/socket.io#501
darrachequesne added a commit that referenced this pull request Jul 8, 2024
darrachequesne added a commit that referenced this pull request Jul 8, 2024
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.

4 participants