WebSocketConnection: make getActiveWebSockets() static

[tius2000]

IMHO getActiveWebSockets() should be declared static. This it allows it to be used with and without an object reference.

BTW: It is marked deprecated, what method should be used instead?

[slaff]

IMHO getActiveWebSockets() should be declared static.

What is the use-case for directly using getActiveWebSockets? If it is to broadcast a message we already have that implemented. getActiveWebSockets gives a powerful access to the websocket instances that can be abused and can easily lead to memory leaks.

[mikee47]

Consider the following scenario:

1. I/O (or other) request received via websocket
2. Request executed asynchronously
3. time passes...
4. Client closes websocket
5. Another client is assigned websocket, WebSocketConnection object coincidentally has same address as ours
6. More time passes...
7. Callback invoked on completion, result sent back to originating websocket

At (7) we need to validate the websocket to (a) ensure it's still valid, and (b) hasn't been reassigned to another client. My solution to this is was to assign a unique CID to the WebSocketConnection (actually an inherited class) at (1), then searching the active websockets list for a matching CID at (7). It uses getActiveWebsockets() do to this. Note that CID cannot be the WebSocketConnection object address.

I'll have a PR for this at some point...

[tius2000]

What is the use-case for directly using getActiveWebSockets?

If it is to broadcast a message we already have that implemented. getActiveWebSockets gives a
powerful access to the websocket instances that can be abused and can easily lead to memory leaks.

In this specific case, it is about the distribution of messages to all clients that have subscribed to a specific message types (publish-subscribe pattern). Broadcast can't be used because only subscribing clients should receive the specific messages. There is no specific object reference in the context of the event that triggers the message generation. The subscription information is stored very efficiently with setUserData(). Managing a separate lists of subscribing clients would be possible, but less efficient.

[mikee47]

@tuis2000 One of my changes is to provide a createConnection() callback in WebsocketResource to allow WebSocketConnection to be overridden. That should make your scenario even easier to implement.

[slaff]

@tius2000 If you want this PR merged in this release, please, rebase it on the latest develop branch. Notice that now WebSocketConnection.h is called WebsocketConnection.h.

[slaff]

I'll have a PR for this at some point...

@mikee47 Do you want to submit your PR related to websockets? There are no major changes in the pipeline from me related to websockets.

[mikee47]

One of my changes is to provide a createConnection() callback in WebsocketResource to allow WebSocketConnection to be overridden.

I've mulled this over and I think my suggestion of overriding WebsocketConnection will just confuse things and won't add anything to the existing method using callbacks. However, there are clear use cases for access to the active websockets list, so I propose un-deprecating it, making it static, but also make it less 'dangerous' by making it return a const WebsocketList.

[slaff]

so I propose un-deprecating it, making it static, but also make it less 'dangerous' by making it return a const WebsocketList.

Sounds good. Send a PR with the proposed changes. Meanwhile I am closing this PR.