fixed:Goroutine Leak Resolution in Relay Server

[KhairnarLokesh]

What does this PR do?

Resolves a critical goroutine leak in the relay server (fixes #22).

Specifically, this PR:

1. Wraps the guest websocket connection registration (r.register <- client in cmd/server/main.go) inside a select block. This prevents guest connection handlers from blocking indefinitely if the host disconnects and the room's event loop has terminated before registration finishes.
2. Wraps the message broadcast channel send (c.room.broadcast <- msg in cmd/server/main.go) inside a similar select block monitoring the room's done channel. This ensures that the guest's read pump goroutine terminates cleanly instead of hanging when a room is destroyed.

Type of change

• Bug fix
• New feature
• Refactor / cleanup
• Docs / CI

Checklist

• go build ./... passes locally
• go vet ./... passes locally
• I've tested the change manually
• I've updated the README if needed

[Copilot]

Pull request overview

This PR addresses a goroutine leak in the relay server by ensuring guest connection handlers and guest read pumps don’t block indefinitely when a room is shutting down and its event loop has stopped receiving on unbuffered channels.

Changes:

• Guard c.room.broadcast <- msg in Client.readPump() with a select that exits when the room is closed (room.done).
• Guard guest registration (r.register <- client) with a select that rejects the join when the room is closed (r.done), instead of blocking forever.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-actions]

Feedback on Code Changes

Bugs and Issues

1. Goroutine Leak Mitigation:

   • The changes correctly address the goroutine leak by using select blocks to monitor the done channel. This ensures that goroutines terminate cleanly when the room is destroyed.
   • However, there is a potential issue in the readPump function. If the done channel is closed, the goroutine will terminate, but the client's connection might not be closed properly. Ensure that the connection is closed in the readPump function when the done channel is closed.

2. Error Handling:

   • The error handling in the handleWebSocket function is improved by using select to monitor the done channel. However, the error messages and close messages could be more descriptive. For example, the close message code 4002 is not a standard WebSocket close code. Consider using a standard code or defining a custom one with a clear description.

Security Issues

1. Connection Handling:

   • Ensure that the connection is closed properly in all error paths. The current implementation closes the connection when the done channel is closed, but it might be beneficial to close the connection in other error paths as well.

2. Message Validation:

   • There is no validation of the incoming messages. Ensure that the messages are validated before they are broadcast to other clients. This can prevent potential issues with malformed messages.

Performance Bottlenecks

1. Channel Operations:

   • The use of select blocks to monitor the done channel is a good practice. However, ensure that the done channel is not blocked by other operations. If the done channel is not properly managed, it can lead to performance bottlenecks.

2. Goroutine Management:

   • The number of goroutines can grow significantly if there are many clients. Ensure that the goroutines are managed properly and that there are no leaks. The current changes help mitigate this, but it's essential to monitor the number of goroutines in production.

Idiomatic Go Conventions

1. Error Handling:

   • The error handling is improved, but ensure that all potential errors are handled properly. For example, the ReadJSON and WriteJSON methods can return errors that should be handled.

2. Code Organization:

   • The code is well-organized, but consider breaking down the handleWebSocket function into smaller functions for better readability and maintainability.

Robust Error Handling

1. Error Propagation:

   • Ensure that errors are propagated properly to the caller. For example, the handleWebSocket function should return an error if the WebSocket upgrade fails.

2. Logging:

   • Add logging to track the flow of the program and to help with debugging. This can be especially useful in production to monitor the behavior of the relay server.

Suggested Changes

1. Close Connection Properly:

   • Ensure that the connection is closed properly in all error paths. For example, in the readPump function, close the connection when the done channel is closed.

2. Descriptive Close Messages:

   • Use descriptive close messages with standard WebSocket close codes or define custom codes with clear descriptions.

3. Message Validation:

   • Add validation for incoming messages to prevent potential issues with malformed messages.

4. Error Handling:

   • Ensure that all potential errors are handled properly. For example, handle errors returned by ReadJSON and WriteJSON methods.

5. Code Organization:

   • Break down the handleWebSocket function into smaller functions for better readability and maintainability.

6. Logging:

   • Add logging to track the flow of the program and to help with debugging. This can be especially useful in production.

Final Thoughts

The changes address the goroutine leak issue effectively. However, ensure that the connection is closed properly in all error paths, use descriptive close messages, validate incoming messages, handle errors properly, organize the code for better readability, and add logging for better debugging and monitoring.

[VishalRaut2106]

Thanks so much for this PR.
Catching that encryption bypass in the CLI is huge—rejecting unencrypted commands instead of falling back to plain text is definitely the right move