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

Proposal: add WakeWithCb(cb func(conn gnet.Conn) error) #188

Open
rdmrcv opened this issue Mar 9, 2021 · 5 comments
Open

Proposal: add WakeWithCb(cb func(conn gnet.Conn) error) #188

rdmrcv opened this issue Mar 9, 2021 · 5 comments
Assignees
Labels
enhancement New feature or request optimization Some small optimizations pending development Requested PR owner to improve code and waiting for the result proposal Proposal for this repo waiting for response waiting for the response from commenter

Comments

@rdmrcv
Copy link
Contributor

rdmrcv commented Mar 9, 2021

I use gnet to write websocket server and it looks very promising, thanks for that framework! :)
But I think it can be event better if there is API to modify gnet.Conn context.

Is your feature request related to a problem? Please describe.
There is no straightforward way to change gnet.Conn context without external mutex.

This feature might be useful if I have a something like connection manager that store and manage connections grouped by topics for example. So if I want to add some metadata to connections contexts (for example - mark all topic connections to do something when that conns will be disconnected) I do not have any straightforward solutions to do that somewhere not inside loops like wake-up loop or ticker loop (timer is not usable here if I use multicore since when OnTick fired there is no any information about loop, and conns do not provide that info too).

Describe the solution you'd like
Add to gnet.Conn method that allow not only wake-up but wake-up and execute something.
It may looks like: WakeCb(cb func(conn gnet.Conn) error).
It should not require much changes - in current Wake method we should not call loopWake, but provided func instead. That allow us to serialize access to conn context without locks.

Describe alternatives you've considered
In Tick event might be provided event-loop index, and in gnet.Conn that loop index might be exposed too. That allow us serialize access in every tick and natively distribute connections via loop index to prevent mutex contention. This solution still require externally managed mutex but at least allow us to reduce contention (for example we can have individual mutex for every event loop and lock for one will not stop entire server).

Additional context
I have long-lived websocket connections that distributed to «topics» (something like websocket pub/sub system).
When connection is arrived I put it to topic connections list (managed separately in separate goroutine), and when I want to send message to topic I iterate over stored conns and AsyncWrite to them.

Sometimes I want to track some connections little bit closer and fire event to external system when that connection stops. So if I have a way to modify its context I can throw away many synchronization works and just fire (pooled) send in OnClosed event if context contains some flag.

@rdmrcv rdmrcv added enhancement New feature or request proposal Proposal for this repo labels Mar 9, 2021
@xscode-auto-reply
Copy link

Thanks for opening a new issue. The team has been notified and will review it as soon as possible.
For urgent issues and priority support, visit https://xscode.com/panjf2000/gnet

@panjf2000 panjf2000 added optimization Some small optimizations pending development Requested PR owner to improve code and waiting for the result labels Mar 10, 2021
@panjf2000
Copy link
Owner

Investigating on this requirement, maybe start developing this feature in the near few days.

@rdmrcv
Copy link
Contributor Author

rdmrcv commented Mar 19, 2021

Hello again! I have drafted initial implementation for my needs. It still WIP (I try to solve my initial needs with this solution and change it when I need).
master...ligser:feature/add-async-execute
Feel free to get some inspiration if you'll found that way as valid. If not — I'll wait until until you present your vision and will use my solution before that happen.

@panjf2000
Copy link
Owner

I'd encourage you to open a PR for this feature since you've already implemented it.

@panjf2000 panjf2000 added the waiting for response waiting for the response from commenter label Mar 23, 2021
@rdmrcv
Copy link
Contributor Author

rdmrcv commented Mar 23, 2021

PR created.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request optimization Some small optimizations pending development Requested PR owner to improve code and waiting for the result proposal Proposal for this repo waiting for response waiting for the response from commenter
Projects
None yet
Development

No branches or pull requests

2 participants