Made it possible to set client context for the listening server too. #93

dkorolev · 2024-08-09T08:54:28Z

Slept over what we introduced in #91 yesterday.

Yes, it will have to be the client's responsibility to be mindful that the client context pointer will persist across connections. The clients for whom this matters should use the onclose handler then.

Also, I realized it can be needed to set the client context before the connection is established, just as the server begins listening. And here is the PR.

The issue ID remains the same, #48.

Thanks,
Dima

Theldus · 2024-08-11T19:44:01Z

Hi @dkorolev,
I'm a bit confused about the usefulness of this PR (and now the previous one too). I expected the context pointer to be bound to a single client, not all clients, and now I realize that in the previous PR it also bound to all clients, which is not exactly what I expected.

I expected the context pointer to be inside struct ws_connection, not struct ws_server. As it stands, this greatly reduces the usability of the feature and is not much more useful than a global pointer for example.

dkorolev · 2024-08-11T20:01:42Z

Got it. Sleeping over it again!

Theldus · 2024-08-11T20:10:32Z

Honestly, maybe we can keep the two approaches, one with a global pointer (like the one in this PR, saving it in struct ws_server) and one saving in struct ws_connection, individual to each client.

dkorolev · 2024-08-11T20:45:09Z

Looks like the cleanest minimum-delta solution is to have two contexts: the client_context and the server_context.

The server_context, if needed, should be initialized by the caller in struct ws_server, prior to the call to ws_socket(). The user is then not expected to wiggle with the server_context.

The client_context is tied to a connection. The user code will be expected to initialize the client_context in onopen, and then the user can use this client context from onmessage and onclose.

Thought experiment: each websocket connection should know its unique autoincremented ID. The variable with the next ID (as well as the mutex guarding it, unless it's an atomic) lives in the server_context. The autoincrement takes place in onopen. The "current connection's index" is then stored in the client_context.

We could call it the connection_context instead of the client context, to avoid confusion.

If you think this is a workable idea, I'd be happy to put it into code.

Theldus · 2024-08-11T21:17:21Z

The server_context, if needed, should be initialized by the caller in struct ws_server, prior to the call to ws_socket(). The user is then not expected to wiggle with the server_context.

Yes, completely agree, something like this is more than enough for me:

ws_socket(&(struct ws_server){
  .host = "localhost",
  .port = 8080,
  .thread_loop   = 0,
  .timeout_ms    = 1000,
  .evs.onopen    = &onopen,
  .evs.onclose   = &onclose,
  .evs.onmessage = &onmessage,
  .context       = my_pointer  // <<<-- here
});

Since struct ws_server is visible to the user, not need to helper functions here either. If one, maybe a function to retrieve the ws_server (via ws_cli_conn_t), not only the context.

The client_context is tied to a connection. The user code will be expected to initialize the client_context in onopen, and then the user can use this client context from onmessage and onclose.

Yes, this partially happens in the last PR (#91), but here saving the context into ws_cli_conn_t, instead of ws_server.

Thought experiment: each websocket connection should know its unique autoincremented ID. The variable with the next ID (as well as the mutex guarding it, unless it's an atomic) lives in the server_context. The autoincrement takes place in onopen. The "current connection's index" is then stored in the client_context.

We could call it the connection_context instead of the client context, to avoid confusion.

If you think this is a workable idea, I'd be happy to put it into code.

I think I understand the idea, but not the objective. I think it is important to keep the server code simple and I don't know if having a connection id would bring any direct benefit.

For me, the ideas of pointers in the server context and connection/client context have a clear benefit, but this last idea not so much.

dkorolev · 2024-08-11T22:15:48Z

Ah, I only meant the incrementing indexes as an example -- and added it too in a separate commit: 5bd34ec

This PR now separates the server context from per-connection contexts. Per-connection contexts are called connection contexts, and the term client_context is now retired.

Theldus · 2024-08-12T00:10:01Z

Hi @dkorolev
Sorry, but I don't want to merge as it is right now: your changes in echo.c are quite confusing and as I said, I don't even understand the need for a connection id. The examples are meant to be simple, and beginner friendly, and I don't see that.

Although targeted for C users, I don't want to introduce/force concepts like threads, mutexes and etc, let alone for the first/simplest example file.

Please undo your changes in echo.c and lets keep the scope only for the context in ws_server and the connection.

dkorolev · 2024-08-12T09:26:43Z

Most certainly. The second commit with an example was mostly a sanity check, plus perhaps to make the code review more enjoyable. Agree the code in the repo looks just fine with echo.c being as simple as possible.

Removed this second commit from the PR, leaving only the change with the context.

For future readers of this thread, the now-removed change to echo.c with the example can be found in this gist, as well as in this commit to a branch in my fork. The latter link may stop working, the former one is hopefully eternal.

Please take another look.

Theldus

Hi @dkorolev,
Thank you very much for the persistence while considering my suggestions, it is now the way I've envisioned =).

Approved.

dkorolev · 2024-08-12T14:33:07Z

Pleasure working with you on this one!

Theldus · 2024-09-05T23:57:05Z

Hi @dkorolev,

Thought experiment: each websocket connection should know its unique autoincremented ID. The variable with the next ID (as well as the mutex guarding it, unless it's an atomic) lives in the server_context. The autoincrement takes place in onopen. The "current connection's index" is then stored in the client_context.

Just a heads up that this was added in #96.

I apologize for not fully understanding the advantages of an ID/UID/Connection ID, but as already discussed in issue #66 (from a year ago, and which I didn't remember), having an ID solves many problems, mainly regarding the lifetime of 'ws_cli_conn_t' and also regarding the certainty of whether a message was actually delivered to the client or not, an issue that had been around for a long time.

Made it possible to set client context for the listening server too.

55b1cd3

Separated the server context from connection context.

075cefe

dkorolev force-pushed the ws_server_client_context branch from 5bd34ec to 075cefe Compare August 12, 2024 09:20

Theldus self-requested a review August 12, 2024 13:11

Theldus approved these changes Aug 12, 2024

View reviewed changes

Theldus merged commit 8c67ca8 into Theldus:master Aug 12, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Made it possible to set client context for the listening server too. #93

Made it possible to set client context for the listening server too. #93

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Made it possible to set client context for the listening server too. #93

Made it possible to set client context for the listening server too. #93

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!