Theldus / wsServer

wsServer - a tiny WebSocket server library written in C
https://theldus.github.io/wsServer
GNU General Public License v3.0
422 stars 80 forks source link

Connection UID #66

Closed CycloneRing closed 6 hours ago

CycloneRing commented 1 year ago

Hi, great code! I wonder if is it possible to make a uid for each connected client for database purpose like websocket-sharp for C#. is is possible? If not can you consider it as a TODO?

Also having a OnError event would be nice.

Theldus commented 1 year ago

Hi @CycloneRing,

It seems interesting, but what is the use case? Since users are 'ephemeral', it seems to me that using the 'fd' of the connection as an ID would be enough, wouldn't it?

About OnError, I haven't added it yet as it seems unnecessary to me. However, to make it useful, adding an error reason, such as 'handshake failure' or 'malformed frame' would be necessary.

CycloneRing commented 1 year ago

@Theldus Thank you for your reply, Currently clientID is the pointer of array, It means if a connection disconnect and someone else connect it get same previous ID which last client disconnected from. Having a UID give better advantage on identifying clients.

Theldus commented 1 year ago

Hi @CycloneRing, Thanks for coming back on this issue.

Yes, you are right, indeed, this is a problem that I have been thinking about how to solve for a long time.

The worst problem with the current approach is that ws_sendframe*() can't guarantee that the message is delivered to the right client, and this is quite problematic, especially if you want to save the clients list (as in #70) or handle clients later by any reason.

Thinking about implementation maybe a simple approach would be:

What do you think about? this shouldn't be too complicated to implement and should solve all the issues mentioned before.

CycloneRing commented 1 year ago

Thank you for your time, It makes sense but why not just using a struct based array instead of actual pointer like struct ClientObject { ws_cli_conn_t clientPtr; uint64_t clientID; int clientIndex; }

should work?

Theldus commented 1 year ago

Hi @CycloneRing, Actually I am referring to what should be passed as a parameter to the on_open (and related) functions and also in ws_sendframe_*(), like in:

void onmessage(<something>, const unsigned char *msg,
    uint64_t size, int type)
{
  ws_sendframe_txt(<something>, "message");
}

So I don't really know how to fit a struct like you proposed: if it's static, I need to reuse it for another client later, if it's dynamic, I need to deallocate it at some point.

In the current code, ws_cli_conn_t refers to an internal wsServer structure, which as you noticed, can change and point to another client later. So dealing with another similar data structure looks like I'm going to repeat the same problem...

CycloneRing commented 1 year ago

Hi @Theldus , I will put some time on this later to see if I can implement it and let you know. Thanks for your dedication.

henriquetft commented 1 week ago

Are you still wanting this feature?

Theldus commented 4 days ago

Hi @henriquetft,

Yes, I still want this feature, especially since it's been requested multiple times, and implementing it could also resolve some current issues, like not being sure if a message was delivered to the correct client, etc.

Regarding the implementation, I believe the approach I suggested here is still valid: making ws_cli_conn_t a uint64_t, which would be incremented atomically with each new connection. The internal routines of wsServer would then iterate over the client list (with appropriate locks) to retrieve (or not) the client corresponding to that UID.

However, this approach does bother me a bit, as it adds an iteration for every operation on the clients, potentially slowing down the server. But then again, wsServer was never really intended to be a blazing-fast server that handles thousands of clients.

What do you think about this?

henriquetft commented 1 day ago

It adds an iteration for each operation, but I don't think that's an issue at the moment unless the dev sets MAX_CLIENTS to a high value. If needed, we can address it later.

I will work no this issue.