mongoose icon indicating copy to clipboard operation
mongoose copied to clipboard

Don't require a connection id to use mg_wakeup

Open jameshilliard opened this issue 1 year ago • 4 comments

Makes broadcasts cleaner I think.

See: #2786

jameshilliard avatar Jun 14 '24 22:06 jameshilliard

Sorry, this breaks the very essence of having this function in the first place. https://mongoose.ws/documentation/tutorials/core/multi-threaded/

scaprile avatar Jun 14 '24 22:06 scaprile

Sorry, this breaks the very essence of having this function in the first place.

Maybe I'm misunderstanding but I thought the purpose of this function is to pass data to the mongoose thread from a separate thread. I'm not sure how this would break anything.

jameshilliard avatar Jun 14 '24 22:06 jameshilliard

@jameshilliard you can send wakeup to the listening connection, if you don't have any specific connection to wake up.

cpq avatar Jul 04 '24 05:07 cpq

you can send wakeup to the listening connection, if you don't have any specific connection to wake up.

I mean, you technically can but it seems to be a lot of excess complexity vs something along these lines, sending to the listing connection for a broadcast like this just seems to be weird to me. You can also see in the multithreading example how this is also a pretty significant reduction in lines of code and cleaner api for broadcast type operations.

jameshilliard avatar Jul 04 '24 05:07 jameshilliard

@jameshilliard if you want to just wake up the manager without sending specific data to a target connection, we could make it by accepting 0 as a target connection id:

https://github.com/cesanta/mongoose/blob/8a8ff2aef0d8f171321100377446767c61ac69fc/src/sock.c#L681

In that condition, just remove "conn_id > 0" part. Would that give what you're looking for?

cpq avatar Oct 06 '24 06:10 cpq

if you want to just wake up the manager without sending specific data to a target connection

Well I do need to send data, just like in the change I made here to the multi-threaded example.

In that condition, just remove "conn_id > 0" part. Would that give what you're looking for?

Wouldn't that result in no event being fired due to the connection filtering in wufn?

When testing I had to change this as well for sending an event to a manager without a specific target connection: https://github.com/cesanta/mongoose/blob/8a8ff2aef0d8f171321100377446767c61ac69fc/src/sock.c#L642

jameshilliard avatar Oct 06 '24 07:10 jameshilliard

@jameshilliard I guess we have different ideas about the usage of it.

Your example is about broadcasting.

Our example about targeting a single connection - imagine that requests to a certain API endpoint require a long calculation, and API endpoint takes some parameters. So every connection starts a thread with some unique parameters - that's why we had struct thread_data which you deleted, cause for broadcasting, it does not matter. And a worker thread generates a response that is unique, and is required for only one connection.

So yeah, we're talking about different use cases.

By accepting 0 as a connection ID, we can do both targeted wakeup, and broadcasts. Yes, we would need to change wufn() too. And no, we can't modify the example code as you suggest, cause it showcases targeted wakeup, not broadcasts.

cpq avatar Oct 06 '24 15:10 cpq

Sorry to interrupt, the second part of our example (one-to-many), IMHO, does just what the OP wants, maybe not the way he'd like to. https://mongoose.ws/documentation/tutorials/core/multi-threaded/#the-one-to-many-case https://github.com/cesanta/mongoose/tree/master/tutorials/core/multi-threaded-12m

scaprile avatar Oct 06 '24 15:10 scaprile

no, we can't modify the example code as you suggest

I don't see what the issue is here, I simply changed the example to demonstrate how the changes I made here are an improvement for broadcast operations, I could separate the example changes by creating a new example if that's preferred.

it showcases targeted wakeup, not broadcasts.

Eh, it's clearly a broadcast example, my changes in this PR significantly simplify these sort of operations as you can see by the fairly significant reduction in overall lines of code of the example by eliminating the pointless(for broadcast operations like this) parent connection id state tracking.

Sorry to interrupt, the second part of our example (one-to-many), IMHO, does just what the OP wants, maybe not the way he'd like to.

Right, the second example is effectively doing the same thing I am trying to do. The issue I have is that the example is unnecessarily complex/hacky due to the API limitations. The unnecessary connection id tracking requirement of the current API becomes particularly problematic if you are wanting to listen on multiple ports from a single manager as well I think. It also seems to effectively make it a requirement that one start the thread calling mg_wakeup from within the event handler which shouldn't actually be necessary as you can see in my changes to the example.

jameshilliard avatar Oct 06 '24 20:10 jameshilliard

Well, I happen to have written that second example, and don't feel the same way about it... :wink:

scaprile avatar Oct 06 '24 20:10 scaprile

don't feel the same way about it... 😉

I've been trying to understand why but I seem to be missing the reasoning behind requiring a connection id for cases when one is obviously not wanted/needed(i.e. broadcasts).

jameshilliard avatar Oct 06 '24 21:10 jameshilliard

Maybe because everything in Mongoose is a connection. You may not want to get a response, but Mongoose needs to identify the endpoint.

scaprile avatar Oct 07 '24 19:10 scaprile

Maybe because everything in Mongoose is a connection.

I mean, this isn't really changing that pattern from what I can tell, it's simply special casing connection id 0 to send the mg_wakeup event to all connections instead of a specific connection.

You may not want to get a response, but Mongoose needs to identify the endpoint.

But it doesn't actually need to identify a specific connection/endpoint for doing a broadcast(as the example changes I made show, this requirement to identify a specific endpoint doesn't really exist in practice), since the actual endpoint one wants to send to is in reality all endpoints/connections.

jameshilliard avatar Oct 07 '24 20:10 jameshilliard