Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switchable server side PONG #2186

Closed
1 task done
tomarigr opened this issue Dec 22, 2023 · 12 comments
Closed
1 task done

Switchable server side PONG #2186

tomarigr opened this issue Dec 22, 2023 · 12 comments

Comments

@tomarigr
Copy link

Is there an existing issue for this?

  • I've searched for any related issues and avoided creating a duplicate issue.

Description

Hello, I know that as per RFC a WS server/client running on this library will automatically pong when a ping is received. While this is great and makes everyone's life easier, sometimes this is not ideal.

A good example is troubleshooting a device/system internet recovery strategy on demand.
As such, I wanted to see what my device will do if a WS ping was not responded and test that my code will try to re-init the LTE modem if it discovers that a certain number of WS pings were not getting a response.

To give some context, it was found that due to some problems the mobile operator randomly is facing, although the connection status remains "connected" and the data channel seems UP, data packets are actually getting lost unless the client disconnects completely from the base station and re-connects.

Although testing this on an ethernet/wifi connected device is easy, just unplug/turn off wifi or the likes, deactivating/re-activating a simcard to simulate network down usually takes time and makes troubleshooting these types of failures a painful time consuming process.

A quick solution was to manually edit the library and add a single if(process.env.BLOCK_PONGS!="on") { on line 1218 of websocket.js and this little hack did the trick for me as I was able to switch auto ponging on an off on demand from my app, or keep it off and handle socket.on('ping' and socket.on ('pong') events and manually .ping() and .pong().

I was thinking that making the automatic pong of this library officially switchable and on by default gives more control to the devs to decide what to do and perhaps chose to manually handle it in the code in some edge cases like mine.

ws version

all

Node.js Version

No response

System

No response

Expected result

No response

Actual result

No response

Attachments

No response

@lpinca
Copy link
Member

lpinca commented Dec 22, 2023

No thanks, I don't see how disabling automatic pong replies is useful. In a test environment you can ignore the 'pong' events. See https://github.com/websockets/ws?tab=readme-ov-file#how-to-detect-and-close-broken-connections.

@tomarigr
Copy link
Author

tomarigr commented Dec 22, 2023

Can you describe how you can make a ws server not pong automatically using the solution you proposed?

Client is not necessarily using this ws library nor a tester necessarily has access to the device fw.

Furthermore, modifying the client fw does not help validation of the real fw for a production environment.

@lpinca
Copy link
Member

lpinca commented Dec 22, 2023

Can you describe how you can make a ws server not pong automatically using the solution you proposed?

You can't (it's a spec requirement), but if you build something that relies on these frames, you can ignore the frames and it would be as if the frame was not sent in the first place.

@tomarigr
Copy link
Author

Exactly my point.

You can't!

I am not building anything, I am validating that third party build embedded devices do what they are supposed to do, using their production intented fw that I have no control of.

Suggestions to modify the production intended fw on the client is not acceptable, especially on strict environments where even a minor change on the fw as simple as deleting a space character requires re-validation.

@lpinca
Copy link
Member

lpinca commented Dec 22, 2023

I understand but I don't want to add an option or an env variable for an edge case like this. You can write a dummy server that does not reply to pings for testing or use another WebSocket library that allows you to do that.

@lpinca
Copy link
Member

lpinca commented Dec 22, 2023

Here is an example:

const crypto = require('crypto');
const http = require('http');

const GUID = '258EAFA5-E914-47DA-95CA-C5AB0DC85B11';

const server = http.createServer();

server.on('upgrade', function (req, socket) {
  const key = crypto
    .createHash('sha1')
    .update(req.headers['sec-websocket-key'] + GUID)
    .digest('base64');

  socket.write(
    [
      'HTTP/1.1 101 Switching Protocols',
      'Upgrade: websocket',
      'Connection: Upgrade',
      `Sec-WebSocket-Accept: ${key}`,
      '\r\n'
    ].join('\r\n')
  );

  socket.resume();
});

server.listen(8080);

This a "black hole" WebSocket server. It establishes a connection and ignores everything else.

@tomarigr
Copy link
Author

Thank you for your suggestions anyway, well it's your choice really.

Can't use a black hole unfortunately, as I want to trigger the loss on demand in various stages. It's a huge test really that test many ways comms loss.

This edge case actually caused thousands of devices in prod going off-line repeatedly over the span of three months, and set fire on vendors trying to fix it.

In any case, I'll keep modding your library because it's the best and I've tried them all, believe me 😅

Can't have it all though, can you?

@lpinca
Copy link
Member

lpinca commented Dec 22, 2023

Another hack that can help you is this

websocket._receiver.removeAllListeners('ping');
websocket._receiver.on('ping', function (data) {
  websocket.emit('ping', data);
})

In this way pongs are not sent.

@lpinca
Copy link
Member

lpinca commented Dec 22, 2023

We could even move this line

websocket.pong(data, !websocket._isServer, NOOP);

to a default listener of the 'ping' event that is added in the WebSocket constructor so that websocket.removeAllListeners('ping') would give you the wanted functionality but I'm not super happy with it.

@tomarigr
Copy link
Author

tomarigr commented Dec 23, 2023

My thought really is, I appreciate this library and always update my modules to latest, and sometimes install them on demand last moment before use via script.

Modding it means I need to keep modding your latest version manually, whereas if there was an official way, I can modify my app to use it and always be up to date.

I don't mind the way it will be done, as long as it is consistent throughout the new releases.

I've demonstrated how I did it quick and dirty by using env variable but would happily bite the bullet if it's done differently by you.

That's why I did this ticket really. To me this lib has it all, and this pong automation is kinda limiting for test/validation use.

Some examples of tests I do are:
Skip the pong immediately after client connection.
Skip 1 pong only (eg, client pings in 10 sec interval but has 30 sec timeout)
Skip all pongs after certain ws messages
Pong back but ignore messages (edge case when a load ballancer handles ws frames but actual server app behind ballancer crashed)
Delay pong but respond to message quickly
Delay message but respond to ping quickly

And many more during other tests that I don't remember off the bat right now.

@lpinca
Copy link
Member

lpinca commented Dec 23, 2023

Using #2186 (comment) works until one day the _receiver property is moved to a real private property. You can get the default listener before calling websocket._receiver.removeAllListeners('ping'); so you can restore it if needed.

@tomarigr
Copy link
Author

Cheers, I'll give it a go!
Merry Christmas!

@lpinca lpinca closed this as completed in 01ba54e Dec 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants