stomp-js / stomp-websocket

Stomp client for Web browsers and node.js apps
https://stomp-js.github.io/stomp-websocket/
Apache License 2.0
141 stars 36 forks source link

Manual disconnect when broker is down #47

Closed ilyabozhkov closed 5 years ago

ilyabozhkov commented 6 years ago

It says here https://github.com/stomp-js/ng2-stompjs/issues/58 that issue is gone, but it persist: if broker or connection is gone and I call disconnect, client keeps PINGs and assumes that it's connected.

Also, update 4.0.2 (2018/02/23) claims that it fixes the issue #21, which is the same issue.

Here is use case: I can detect when computer just woke up from sleeping right away. I know for a fact that the connection to the broker is gone, simply because computer was asleep just before. I want to reconnect right in this moment. (In fact, I have to reconnect).

However, even if I call "client.disconnect" it takes then up to 10 seconds (three retries with PING) to "discover" that connection is actually gone. That's about 10 seconds after another piece of code knows that connection is gone.

If I want to reconnect right away, when I know that connection is gone, what can/should I do?

Calling client.connect doesn't work either until that "surprise" discovery 10-15 seconds later that connection is gone.

Is there any way to tell the client that "connection is gone, accept that fact and stop being in the dark"?

I managed to stop PINGs and attempts to reconnect with the following:

client.connected = false; client._cleanUp();

(and peacefully client.connect() reconnect at that time)

However, after a while some crazy PONG comes from the broker (presumably checking and invalidating the previous connection, which is 100% known to be gone, breaks the new connection, makes the client to reconnect yet again. (...making user experience kind of a nightmare as obviously I can't ack "old" messages on the new channel as broker gives me delivery tags that effective only in the original connection/channel, which makes me to reload/init the data layer each time, leaving user waiting.)

kum-deepak commented 6 years ago

This indeed has been a frustrating area for me as well. First a bit of explanation 😄

There are two forms of disconnects:

Let us consider the case of OS waking from sleep. As per WebSocket and TCP specifications, OS sleep does not necessarily causes network connections to be terminated. In reality if we have control over both server and client, the connections may survive even when no data is exchanged for days.

So, the OS is correct in not automatically considering the underlying WebSocket to be dead.

Let us come to possible workaround in this situation. Just setting client.connected = false; may not work as it does not cleanup the Pinger/Ponger. For unit tests I am using client.ws.close(); as a means to force close the underlying WebSocket channel. This should automatically trigger re-connection, however, after reconnect_delay. The reconnect_delay can be kept quite small. I typically set it to 200ms. However it can be kept even smaller. (see: https://github.com/stomp-js/stomp-websocket/blob/master/tests/unit/reconnect.js#L21)

I will definitely be interested in knowing your results.

Due to these and other limitations, I am now in process of rebuilding the code-base bottom up, please join at https://github.com/stomp-js/stompjs/issues/2 - it has more life-cycle callbacks among other changes. I am, now, tempted to add a force disconnect option.

ilyabozhkov commented 6 years ago

Kum-Deepak, thank you for all the work you do and taking care of the package.

client.ws.close(); throws an error something along the lines of "Socket is already CLOSED or in a CLOSING state". I tried it yesterday. I'll double check today.

client._cleanUp(); cleans up the Pinger and Ponger.

Anyway, when the PONG from the broker's "old" connection comes - it breaks the new connection. It should be a way to ignore this PONG, especially after the forceDisconnect

kum-deepak commented 6 years ago

If we just cleanup and the older WebSocket is not actually closed we might have - at least for a short while - two WebSockets. So, I will suggest to ensure that older is closed before opening a new one.

Would you mind trying:

   // https://developer.mozilla.org/en-US/docs/Web/API/WebSocket/readyState
   if (client.ws.readyState === 0 || client.ws.readyState === 1) { 
       client.ws.close();
   }

Alternatively if you just want to ignore all packets received on the older WebSocket, try setting the onmessage to no-op. This will ignore anything received on older WebSocket.

client.ws.onmessage = function(event) {};

Please let me know if either of the above works.

kum-deepak commented 6 years ago

It will be also wise to check for client.ws itself before calling other methods on it. It may not be there depending of sequence of calls.

kum-deepak commented 5 years ago

Probably fixed in 5.0.0. Please upgrade https://stomp-js.github.io/stompjs/additional-documentation/upgrading.html