search

Flotype / now

NowJS makes it easy to build real-time web apps using JavaScript
http://www.nowjs.com
MIT License
1.91k stars 175 forks source link

[XHR-Multipart] Server crash when client now.functions aren't at the top. Mostly FF. #92

Closed toqueteos closed 13 years ago

toqueteos commented 13 years ago

This breaks the server when a XHR-Multipart client joins (Firefox 3.x or 4.x):

<script>
  $(document).ready(function() {
    now.name = prompt("What's your name?", "");
    now.receiveMessage = function(name, message) {
      $("#messages").append("<br>" + name + ": " + message);
    }

    // ...
  }
</script>

Solution: Move the function declaration to the top and error will get fixed magically.

This must be mentioned on the wiki, examples should be fixed, etc... I can do a pull if you want. It's not a big deal.

I've tested this with a modified version of the multiroom example on Chrome 11.0.696.71, FF 4.0.1, Steam's InGame Web browser and IE9 (IE9 actually did weird things).

team's InGame Web browser issues:

  1. Prompts seem to be disabled so..
  2. Connect with xhr-polling.
  3. Works very nice (name is null).

This should be mentioned too. Provide some kind of fallback version with forms to login or add a change your name feature inside the chat, but "null has joined messages" will be annoying. Anyway, your decision.

What IE9 did:

  1. Connect with xhr-polling.
  2. Also connect jsonp-polling.
  3. Drop the xhr-polling connection.
  4. Worked nicely for a while (1~2 minutes). Me: * surprise * Yay it "works"
  5. Drop the jsonp-polling connection.
  6. Me: ???
  7. Report Issue.
toqueteos commented 13 years ago

Oh, I forgot Steam's User Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; Valve Steam GameOverlay; ) AppleWebKit/534.1 (KHTML, like Gecko) Chrome/6.0.444.0 Safari/534.1

My Chrome UA: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/534.24 (KHTML, like Gecko) Chrome/11.0.696.71 Safari/534.24

It seems it's chromium under the covers...

ericz commented 13 years ago

Thanks for the excellent detailed bug report. I haven't tested with Steam's browser before but I'll check it out. The issues you mention sound like largely socket.io feature detection problems though, and may be out of scope of what we can do to fix

tommedema commented 13 years ago

@ericz, have you figured out how it can cause a crash on the server side (which is the case according to the OP)?

ericz commented 13 years ago

@tommedema @dvv Hey guys I merged toqueteos patch to work around the issue but I definitely prioritize fixing any bugs that cause a possible server crash.

@toqueteos, Can you provide the console output when the server crashes from this issue?

Thanks, Eric

toqueteos commented 13 years ago

@ericz, sure. There you go:

root@rvb:/home/ratnu/_node# node server.js 
2 Jun 23:32:34 - socket.io ready - accepting connections
2 Jun 23:32:53 - Initializing client with transport "websocket"
2 Jun 23:32:53 - Client 7478356582578272 connected
Joined: toqueteos chrome
2 Jun 23:33:23 - Initializing client with transport "xhr-multipart"
2 Jun 23:33:23 - Client 6012892031576484 connected
2 Jun 23:33:23 - xhr-multipart message handler error - TypeError: Object #<Object> has no method 'receiveMessage'
    at Object.<anonymous> (/home/ratnu/_node/server.js:56:14)
    at Object.<anonymous> (native)
    at Object.<anonymous> (/home/ratnu/_node/server.js:18:12)
    at Object.<anonymous> (events.js:64:17)
    at EventEmitter.addUser (/usr/local/lib/node_modules/now/lib/clientGroup.js:118:17)
    at Object.createScope (/usr/local/lib/node_modules/now/lib/nowServerLib.js:112:16)
    at [object Object].<anonymous> (/usr/local/lib/node_modules/now/lib/nowServerLib.js:271:46)
    at [object Object].emit (events.js:64:17)
    at [object Object]._onMessage (/usr/local/lib/node_modules/now/node_modules/socket.io/lib/socket.io/client.js:58:10)
    at IncomingMessage.<anonymous> (/usr/local/lib/node_modules/now/node_modules/socket.io/lib/socket.io/transports/xhr-multipart.js:48:16)

Hope it helps.

ericz commented 13 years ago

Hmm this should not be crashing the server but rather only disconnecting the erroring client?

toqueteos commented 13 years ago

@ericz I've testing some more.

It seems server crash it's not the right word, maybe massive client drop is a better one.

Whenever that error ocurrs, the FF client doesn't connect (obvious) and all the other clients that were connected are suddenly disconnected but the server does not handle the disconnect event.

(Chrome)

Sorry for the confusion.

ericz commented 13 years ago

Thanks for reporting.

This is still not ideal behavior so I'll definitely look into it.

Eric

On Fri, Jun 3, 2011 at 5:18 AM, toqueteos < reply@reply.github.com>wrote:

@ericz I've testing some more.

It seems server crash it's not the right word, maybe massive client drop is a better one.

Whenever that error ocurrs, the FF client doesn't connect (obvious) and all the other clients that were connected are suddenly disconnected but the server does not handle the disconnect event.

In that example only if I refresh the page in Chrome I can enter the rooms, the previous "session" is lost, so good news, server doesn't crash, but FF users are a pain in the ass. If i don't refresh, I can chat, but no one will get those messages.

Sorry for the confusion.

Reply to this email directly or view it on GitHub: https://github.com/Flotype/now/issues/92#comment_1296376

510-691-3951 EECS Student at UC Berkeley http://ericzhang.com

ericz commented 13 years ago

0.7 of NowJS now uses 0.7.7 of socket.io so this issue is no longer around. There are still bugs (which are known and being fixed) with concurrent xhr-polling / jsonp polling when having multiple instances of now on IE but this specific issue is no longer reproducible. Closing