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

Websocket client and hub improvements #194

Closed
wants to merge 2 commits into from
Closed

Conversation

karlromets
Copy link
Collaborator

some ideas to further improve websockets

  • added hub parameter to ServerWs in client.go and made the stop channel buffered. This should help with client tracking and stopping

  • renamed run to Run for hub.go and added a non-blocking stop signal when unregistering clients, it should prevent client leaks

  • initialize hub in ServerWs in main.go,

  • fix(api): prevent client leaks, improve hub|

- Register clients to track them
- Use buffered channel to stop clients
- Make hub stop function public
@karlromets karlromets self-assigned this Feb 23, 2025
@karlromets karlromets requested a review from joshzcold February 23, 2025 13:15
Copy link

github-actions bot commented Feb 23, 2025

Test Results

27 tests   27 ✅  54s ⏱️
 8 suites   0 💤
 1 files     0 ❌

Results for commit f446ed4.

♻️ This comment has been updated with latest results.

Copy link
Owner

@joshzcold joshzcold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just not sure I understand the goal of this change?

I would think each room would want 1 hub and I don't know why we would want a globally initialized hub?

@@ -50,8 +50,11 @@ func main() {
log.Panicf("Error: unable initalize store: %s", err)
}

hub := api.NewHub()
go hub.Run()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this make it so every client is connect to the same hub?

Im not sure I understand the purpose?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, totally my blunder, completely misunderstood here...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, thank you 🙏

@karlromets karlromets closed this Feb 24, 2025
@karlromets karlromets deleted the fix/websocket-leaks branch February 28, 2025 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants