Cannot disconnect a socket if the initial connection wasn't established

[zalmoxisus]

Though the title sounds bizarre, the issue can be reproduced as following:

1. Try to connect to a server that is not available with socket.connect().
2. Disconnect the socket via socket.disconnect().
3. Connect to a live server with socket.connect().

It works well, but it still tries to reconnect to the first not available server every second and throw SocketProtocolError with the message Socket hung up. Should we somehow destroy it?

[jondubois]

@zalmoxisus Note that var socket = socketCluster.connect(options) is not the same as socket.connect() - In the second case, the connect function doesn't accept any arguments. You cannot change a socket's host - If you want to connect to a different host, you need to create a new socket using socketCluster.connect(options).

Look at the connect function in http://socketcluster.io/#!/docs/api-socketcluster-client vs http://socketcluster.io/#!/docs/api-scsocket-client

Let me know if this is a different issue.

[zalmoxisus]

Didn't know there are two of them. I meant socketCluster.connect(options) in both cases. Here's my function I use:

import socketCluster from 'socketcluster-client';
var  socket; // Global for my module
export function subscribe(options) {
  if (socket) socket.disconnect();
  socket = socketCluster.connect(options);
  // ...
}

When I specify a wrong port, then call again with a correct one, it still retries the first attempt though it disconnected that one and new connection works properly.

[jondubois]

@zalmoxisus What version of the client are you using? socketCluster.version

[jondubois]

Nevermind, I was able to reproduce. Thanks.

[zalmoxisus]

@jondubois, at first I was thinking the fix for the reconnecting in the last release was causing this, but downgrading didn't help.

[jondubois]

@zalmoxisus This behaviour is somewhat expected when you have the autoReconnect option set to true (which is the default) - SC will try to reconnect the client even if it was never connected in the first place.

Ideally, you should be able to listen to a 'connectAbort' event on the socket, then you could manually call socket.disconnect() to prevent any further reconnection attempts... Or you could keep creating new sockets until you find a port that does work and THEN call disconnect() on all the sockets which failed.

^ I did find a bug which prevents this approach from working right now though - When the connection fails initially, the socket.state will be set to 'closed' and it will wait a bit before trying to reconnect (at which point the socket.state will be set to 'connecting' again). If you call socket.disconnect() while the socket is in a 'closed' state, it will not cancel the pending reconnection attempt (which is incorrect).

Another different but related issue I found is that it takes a long time before the client gives up on connection attempts (currently, the browser is deciding when to timeout the WebSocket connection) - Instead, it should give up after a user-defined timeout is reached. Maybe we need a new option like connectTimeout on the client?

I will try to fix this ASAP.

[zalmoxisus]

@jondubois, thanks a lot for the detailed explanation and your work here! I thought the 'closed' state was the result of socket.disconnect().

The current scenario of trying to reconnect even if it was never connected is justified as the server can become available. Just making socket.disconnect to clear reconnecting timeouts would be the solution. That's not a high priority issue as usually apps don't let users to configure which socket server to use as we do for remote-redux-devtools.

[jondubois]

@zalmoxisus This was fixed in socketcluster-client v4.3.3.

Remember to call disconnect() on the socket if you want to socket to stop retying.

[zalmoxisus]

Thanks! Works as intended now.

Other thing that I noticed is if we have for example

socket = socketCluster.connect(options);
socket.emit('login', {}, () => {});

It tries to login even though the connection wasn't established (and throws TimeoutError with "Event response for 'login' timed out").

Shouldn't it emit events only after the success (re)connecting?

[jondubois]

@zalmoxisus That is expected behavior. SC doesn't buffer messages for longer than timeout. When you emit before the connection is made, SC will try to connect and then send the message to the server. If however, the connection cannot be established before ackTimeout is reached, then the message will be dropped and an error will be sent back - You can handle this error however you like. There are too many ways to handle these errors so it is outside the scope of SC how you want to do error handling.

SC delivers messages on a best attempt basis, if you want better delivery guarantees, you will have to add our own logic to deal with this. In a typical frontend scenario, you might use such timeout errors to inform the user that the message could not be delivered because their internet was down.

[zalmoxisus]

Got it, thanks! I guess the best way is to wrap everything in a socket.on('connect', fn).