Skip to content

Loading…

xhr.abort on disconnect not sending connection close to server on beforeunload event #461

Closed
sanjayk opened this Issue · 35 comments
@sanjayk

Is any body else running into this issue?

Here is the scenario:

  1. We are running node/socket.io on a separate server with its own DNS/domainname (Don't ask why) and our application server is on a different host/DNS/domainname.
  2. The socket.io.js file is rendered from the node server.
  3. When a user navigates away from a page we would like to send a disconnect event to the server from client. (Should be pretty easy right?)
  4. Upon debugging we found that on line 3436 in socket.io.js an xhr.abort command is issued which should tell the server to initiate a disconnect event. (But no dice).
  5. The disconnect finally fires but not from the unload event, but from the connection timeout.
  6. Seems pretty simple, anyone else having these issues?

Thanks.

@sanjayk

As much I don't like the solution and was hoping someone would provide a more "elegant" solution, at this time just sending an synchronous disconnect on the window unload event does the trick but that is a definitely a hack job solution.

@quahada

I'm having the same issue.

I noticed that the disconnect happens on timeout when using xhr-polling, but does fire on reload when using websockets.

However, I tried creating a debug message to further find the problem:
client:
$(window).on('beforeunload', function(){

socket.emit('leaving-page');
});

server:
socket.on('leaving-page', function(){
console.log('leaving-page');
});

This command is executed when using websockets, but not when using xhr-polling. (relatively new to node/socket.io, so I'm not sure if this is obvious).

Can you explain the synchronous disconnect event, b/c I haven't been able to get the disconnect message to manually fire on reload.

thanks!

@quahada

*using socket.io v0.9.6

@rauchg

@sanjayk would you mind putting together a testcase ?

@crickeys

I have this issue as well. I found this out because I had to disable websockets because they cause a memory leak that makes socket.io unusable in production with websockets (so surprised more people aren't complaining about this). So, I tried to only use xhr-polling which get's rid of the memory leak, but won't close the connection when people navigate from page to page. Instead it has to wait for the close timeout to fire. socketio/socket.io-client#431

@crickeys

Looking over socket.io client, I see that the unload event calls disconnectSync() as opposed to just disconnect(). DisconnectSync seems to send a url as a GET whereas disconnect() sends a POST with a {type: 'disconnect'} which eventually converts to 0::

It doesn't appear that the server interprets the URL sent via disconnectSync() as a disconnect.

@quahada

Can anyone comment on why the custom message I created in my test case doesn't register when the browser refreshes? This doesn't seem to work for either xhr-polling or websockets.

@ruimarinho

I'm having an issue with disconnectSync too and I have no intention to hijack this thread - please correct me if I should open a new issue about this, but it seems related to me.

Firstly, the code mentions a sync XHR, but the XMLHttpRequest is opened with the async option as true (see this line).

I've also noticed that at least when using WebSockets, this.resource returns 'undefined'. Shouldn't this be this.options.resource? It also doesn't take into account the other socket options (scheme, host, port, etc).

Lastly, the change from beforeunload to unload (see here) broke the disconnect event on Chrome for me (v20, OSX).

I'm unable to get a proper disconnect event on the server side, even when sync disconnect on unload is set to true. Using 0.9.6.

@crickeys

Okay, I stepped through all different tags of node. 0.9.4 worked, then 0.9.5 didn't. I narrowed it down to this patch: 2075307

That patch kills this functionality. I don't know why yet.

@crickeys

I don't understand the code well enough to know how to fix this. @guille ?

@darklrd

Even I am facing this issue in 0.9.6

@redannick

Also having trouble with this. Have reverted to 0.9.4 and it's working as @crickeys mentions

@darklrd

Yup, it works fine in 0.9.4.

@crickeys

@guille I know you are busy with 1.0 but until that is stable (which might be a long time) can we get this issue fixed? I narrowed it down as much as possible but because websockets have memory leaks I'm forced to only use xhr-polling in production. And, with this bug you simply can't use it effectively because you have to wait for timeouts to fire before a visitor disconnects. Think you could take a quick look here 2075307

@rauchg

@crickeys you're missing the ?disconnect query string portion. Also I just noticed the bad indentation, I'm gonna fix this

https://github.com/LearnBoost/socket.io/blob/master/lib/manager.js#L642

@rauchg

Keep in mind to use spaces instead of tabs next time

@crickeys

I see... then perhaps the commented out line in socket.io should be added in now. I didn't fully understand the socket.io protocol on this one, I just thought the change you originally added here (socketio/socket.io-client@e6e34d4) was a little malformed and missing the send.

My editor is set to replace tabs with 4 spaces, so I'm not sure where I messed up on that one.

@crickeys

@guille Even with the ?disconnect in the query I still don't see it working properly without that line removed in socket.io http-polling.js (f48b40e)

I tried with the latest socket.io-client and without my fix which removes line 55 of http-polling.js it just doesn't seem to register the disconnect. Any ideas?

@rauchg

New socket.io release with this fix in a few minutes.

@rauchg

Stay "connected". wat

@rauchg

0.9.9 should fix this

@rauchg rauchg closed this
@crickeys

That seemed to do the trick! Note, you need both changes in socket.io-client AS WELL as the ones here in socket.io for it to work. Nice job @guille !

@scanales

thanks! @guille

@ngzhongcai

Not sure if related...
Added "sync disconnect on unload"= true on both client and server-side.
Works ok on the browsers on my laptop.
Not ok on my iPad though.

Npm the latest version of socket.io. Not sure if my client is on 0.9.9 though

@scanales

Gotta update the client-side script as well.

@ngzhongcai
@scanales

@ngzhongcai that's correct, I don't serve the client-side script from the socket.io server because I load several stuff at the same time, so I had to do it manually

@utan

@crickeys I have same issue with new socket.io 0.9.14 and commenting the line you referred fixed the bug..
What was the fix for this ?

regards.

@kamend

Sorry to open this again, but I have the same problem, using Socket.IO 0.9.16 and Mobile Safari on iOS 7.0.4 with XHR-Polling. Commenting out line 55 in http-polling.js, fixed the issue:

HTTPPolling.prototype.setHandlers = function () {
  HTTPTransport.prototype.setHandlers.call(this);
  this.socket.removeListener('end', this.bound.end);
  //this.socket.removeListener('close', this.bound.close);
}; 
@paulofelisbino

@kamend, same here - Socket.IO 0.9.16 and IE 9. That solution works.

@Bl0tCh

Why has this fix been reverted then (1fa74a4) ? is it safe to apply ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.