SocketCluster / socketcluster

Highly scalable realtime pub/sub and RPC framework
https://socketcluster.io
MIT License
6.14k stars 313 forks source link

socketcluster compliance #595

Open SSANSH opened 9 months ago

SSANSH commented 9 months ago

I work with a server in version socketcluster-server@19.0.1 (why its not available on tags of repository). And a client on version socketcluster-client@14.3.2

I have issue with following code :

node_modules/socketcluster-server/serversocket.js

` // Receive incoming raw messages this.socket.on('message', async (messageBuffer, isBinary) => { let message = isBinary ? messageBuffer : messageBuffer.toString(); this.inboundReceivedMessageCount++;

let isPong = message === pongMessage;

`

problem is that isBinary function is not implemented on ws link to the version of the client socketcluster-client@14.3.2 Thats means when pong arrive isBinary is set to false and we cannot validate the condition let isPong = message === pongMessage;

What to you think to change this code by :

node_modules/socketcluster-server/serversocket.js

` // Receive incoming raw messages this.socket.on('message', async (messageBuffer) => { let message = Buffer.from(messageBuffer).toString(); this.inboundReceivedMessageCount++;

let isPong = message === pongMessage;

` This will work on all case with and without isBinary.

or other option is to specify an option to keep the compliance with old client by running this code conditionnaly.

I know the best options is to update server and client, I will do this however this will be more easy to keep compliance during migration.

jondubois commented 9 months ago

Thanks for this thorough description. Looking at the suggested change, the functionality appears to be a bit different because it will always convert to a string. Currently, it will sometimes keep the message as a buffer (binary) and not a string. It's important that SC continue to support raw binary (even though that's an unusual use case).

About your problem. Since you are using different versions of the SC client and server, I would expect your package manager (e.g. npm or yarn) to point to two different versions of the ws module for the client and for the server. I would suggest looking into why the client and server are sharing the same version of the ws module/dependency. Normally npm can handle this case without issue. Maybe something to do with bundling step de-duplicating the dependency?

SSANSH commented 9 months ago

Thanks too for your quick answer my ws dependency is setup like this.

` ├─┬ socketcluster-client@14.3.2 │ └── ws@7.5.9

├─┬ socketcluster-server@19.0.1 └── ws@8.14.2 `

Client and server actualy not share the same version of ws.

note my issue is only on ping pong process.

I understand the raw thing maybe you can restrict the change like this

serversocket.js 
134d133
<     let messagePong = Buffer.from(messageBuffer).toString();
137c136
<     let isPong = messagePong === pongMessage;
---
>     let isPong = message === pongMessage;

in order to identify each time the ping correctly

jondubois commented 9 months ago

@SSANSH OK. You could consider either downgrading socketcluster-client to an older version which uses an older ws module or upgrading it. Also, did you set the protocolVersion to 1 on the server? Like this: https://github.com/SocketCluster/socketcluster#compatibility-mode

You need to do this if using an older client with a new version of the server as the new server versions default to protocolVersion 2 which old clients don't fully understand (the difference is related to the ping format so could be related to your issue).