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

connection disconects, when broweer is blocking javavascript (minor issue) #97

Closed shimondoodkin closed 13 years ago

shimondoodkin commented 13 years ago

with the now simple chat example:

when the prompt enter your name shows up- then you wait long time before clicking ok, it disconnects (you see it in the server). then when you enter the name and click ok, it is not working.

possible solution: to reconnect with same id.

3rd-Eden commented 13 years ago

The issue is related https://github.com/LearnBoost/Socket.IO/issues/128

Solution, don't use prompt, alert or confirm in your apps. Or only do it after you connected to your server ;)

sridatta commented 13 years ago

We really should consider getting rid of that prompt from the example code

tommedema commented 13 years ago

Unfortunately this issue is not solved by simply not using prompts, as there are other reasons as well, such as a Flash File Browser.

This is why we need reconnection support (which should be relatively easy with socket.io 0.7). :)

Update: latest 0.6 instead of 0.7, thanks dvv.

dvv commented 13 years ago

Reconnection is done in latest 0.6

ericz commented 13 years ago

Yep reconnection is done but since reconnection occurs with a different clientId the server doesn't know that it's the same client and state is lost.

Here's the tricky part: If we keep the client state around and then attempt to preserve the clientId across reconnects, (we have actually internally built this implementation but have not released yet) then you are creating a memory leak as the server will have to not delete the state of disconnected clients in the chance that the client may reconnect.

One solution is to have a timeout so if a client doesn't reconnect within X seconds then they are deleted.

Thoughts on this? @dvv @tommedema

tommedema commented 13 years ago

@ericz, that is a perfectly acceptable situation and the only real solution as far as I know. Make sure you allow setting the timeout as an option.

One thing though, you should have an internal server-side function, that can be called by the client, like now.core.disconnect() and then listen for the browser event fired when the user exits the page. This should work:

window.onbeforeunload = function() {
    now.core.disconnect();
};

This makes sure that you will never have to keep states of clients that properly disconnected, and only need to keep states of clients that are very likely to reconnect.

Or, better, to make sure that previous callbacks remain:

var priorCb = window.onbeforeunload;
window.onbeforeunload = function() {
    if (priorCb) priorCb.apply(this, arguments);
    now.core.disconnect();
};

Note: this is assuming that the browser has not unloaded when this cb is called, which I believe to be the case. But since I never used this method before you should double-check before you waist on your time on implementing this internal function!

Maybe @dvv has some more feedback.

ericz commented 13 years ago

Yep. Good note. It seems like the clear consensus that a timeout is the only option so we'll go down this path this week.

tommedema commented 13 years ago

@ericz, I have updated my previous post, it now keeps in mind that this callback may already have been set.

dvv commented 13 years ago

Personally, I'd flush the state to DB at server-side. Be it a cookie session (size limit), or redis. The best imho would be if NowJS provided it transparently OOTB. Given that, reconnect is simply treated as vanilla fresh connect, which simplifies things greatly.

tommedema commented 13 years ago

@dvv, that could complicate matters. For example, I have a list of clients each with a reference to their now and client scope. If you were to move this client to a database and later on reset it with the same state, the references will have changed.

However, if you keep the state in memory, nothing changes -- and since a reconnection is generally done within seconds, this should not conserve more memory than if there would not be a disconnection at all, combined with the fact that normal disconnects will immediately flush the client (due to the great onbeforeupload mechanic I described above).

3rd-Eden commented 13 years ago

yes old code

On Jun 4, 2011, at 8:07 PM, sridatta wrote:

We really should consider getting rid of that prompt from the example code

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

Arnout Kazemier info@3rd-Eden.com

dvv commented 13 years ago

@tommedema: "is generally" is the key. I'm glad if this assumption fits your app pattern. But unless reconnection is done within seconds, memory is eaten. I tend to look at NowJS state (future, maybe?) as at a vanilla JS object -- methods in prototype (and as invariants they are instantiated afresh upon connect/reconnect), data members in a store, be it memory, or external DB. Such separation would allow to truly code reuse -- you just require('your prototypes') at server-side and Githubissues.

  • Githubissues is a development platform for aggregating issues.