Fix: Removing clients data when client disconnect during authentication process

[dawidpawlowski]

There is a problem with removing clients data from clients and clientsCount variables in scserver.js. When authEngine is set and authentication process takes some time, user can refresh site or just close it until authentication process finished.
When this situation occur, client data stays on the server and due to this we have wrong information about clients connected to the server.

[jondubois]

@dawidpawlowski Nice find! Also your test case is much appreciated.

This fix is an improvement but it does change the behaviour of the clients object/hashmap in a potentially breaking way.

Before this fix, the clients property would only contain sockets which have completed the authentication step and their state was always 'open'. Now it's possible that some sockets will be in a 'connecting' state.

A common use case for the clients object is to iterate over it and to send messages to those clients. If the socket hasn't completed the authentication check, and the server sends a message to the client it may change the behaviour a bit if the client responds back too quickly (and if the authentication process is slow).

Also if the socket disconnects before the authentication completes, it will still trigger a 'connect' event in this case which doesn't make sense; in this case we should just ignore it (I guess that's another issue with the current code).

Maybe instead of moving the clients and clientsCount outside of the block, we can add some code above this line

socketcluster-server/scserver.js

Line 495 in 7b11cf0

var status = {

which checks if scSocket.state is closed; and if so, return; (so that it doesn't continue to execute).

Is it important for you to have access to sockets before the auth check step has completed?
If so, I think we could add another object/hashmap like clients; maybe called pendingClients which only contains pending clients (excluding open/connected ones or disconnected ones)?

I prefer not to encourage developers to interact with sockets on the back end before the sockets have completed the full handshake (since the authentication is technically part of the handshake). But it's OK if it's a completely separate hashmap.

[jondubois]

Since this is a pretty important fix, I merged it and will make adjustments. Thank you for the contribution.

[jondubois]

@dawidpawlowski I've pushed the fix with additional changes on top. See https://github.com/SocketCluster/socketcluster/releases/tag/v9.1.1

[dawidpawlowski]

@jondubois Thank you for your fixes and involvement in this issue. Now works good :)