Add solution for Challenge 8 by aruncs#1894
Conversation
WalkthroughAdds a Go Challenge 8 chat server template with channel-based clients, synchronized connection management, broadcast and private message routing, and exported error values. ChangesChat server implementation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Sender
participant ChatServer
participant Recipient
Sender->>ChatServer: Broadcast or PrivateMessage
ChatServer->>Recipient: Send Message to inChannel
Recipient->>Recipient: Receive message
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
challenge-8/submissions/aruncs/solution-template.go (2)
10-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant
Messagestruct.Since
Receive()strips all metadata and only returns the message body string, theSenderandReceiverfields add no value. You can simplify the codebase by usingchan stringdirectly for message passing.
67-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate
Clientinitialization to reflect channel changes.Update the struct initialization to remove the unused
outChanneland changeinChanneltochan string, aligning with the suggestedClientrefactor above.♻️ Proposed refactor
client := &Client{ username: username, - inChannel: make(chan Message, 10), - outChannel: make(chan Message, 10), + inChannel: make(chan string, 10), connected: true, }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 124c07ce-390e-4446-b0a5-0aa38f9daf3e
📒 Files selected for processing (1)
challenge-8/submissions/aruncs/solution-template.go
| // Client represents a connected chat client | ||
| type Client struct { | ||
| // TODO: Implement this struct | ||
| // Hint: username, message channel, mutex, disconnected flag | ||
| username string | ||
| outChannel chan Message | ||
| inChannel chan Message | ||
| connected bool | ||
| } | ||
|
|
||
| // Send sends a message to the client | ||
| func (c *Client) Send(message string) { | ||
| // TODO: Implement this method | ||
| // Hint: thread-safe, non-blocking send | ||
|
|
||
| } | ||
|
|
||
| // Receive returns the next message for the client (blocking) | ||
| func (c *Client) Receive() string { | ||
| // TODO: Implement this method | ||
| // Hint: read from channel, handle closed channel | ||
| message := <-c.inChannel | ||
| return message.Message | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Implement thread-safe Send and fix Receive channel handling.
There are several critical functional and stability issues here:
Sendis entirely unwritten, meaning messages are never dispatched through the client's public API.- The
Clientstruct lacks a mutex, leaving theconnectedflag vulnerable to data races. outChannelis declared but never utilized.Receivedoes not verify if the channel is closed, which can cause erratic behavior on disconnect.
Refactor the client to use a single chan string, a mutex, and a non-blocking Send method.
🛠️ Proposed refactor for Client
// Client represents a connected chat client
type Client struct {
- // TODO: Implement this struct
- // Hint: username, message channel, mutex, disconnected flag
username string
- outChannel chan Message
- inChannel chan Message
+ inChannel chan string
+ mu sync.Mutex
connected bool
}
// Send sends a message to the client
func (c *Client) Send(message string) {
- // TODO: Implement this method
- // Hint: thread-safe, non-blocking send
-
+ c.mu.Lock()
+ defer c.mu.Unlock()
+ if !c.connected {
+ return
+ }
+ select {
+ case c.inChannel <- message:
+ default:
+ // Channel full, drop message to prevent blocking
+ }
}
// Receive returns the next message for the client (blocking)
func (c *Client) Receive() string {
- // TODO: Implement this method
- // Hint: read from channel, handle closed channel
- message := <-c.inChannel
- return message.Message
+ msg, ok := <-c.inChannel
+ if !ok {
+ return ""
+ }
+ return msg
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Client represents a connected chat client | |
| type Client struct { | |
| // TODO: Implement this struct | |
| // Hint: username, message channel, mutex, disconnected flag | |
| username string | |
| outChannel chan Message | |
| inChannel chan Message | |
| connected bool | |
| } | |
| // Send sends a message to the client | |
| func (c *Client) Send(message string) { | |
| // TODO: Implement this method | |
| // Hint: thread-safe, non-blocking send | |
| } | |
| // Receive returns the next message for the client (blocking) | |
| func (c *Client) Receive() string { | |
| // TODO: Implement this method | |
| // Hint: read from channel, handle closed channel | |
| message := <-c.inChannel | |
| return message.Message | |
| } | |
| // Client represents a connected chat client | |
| type Client struct { | |
| username string | |
| inChannel chan string | |
| mu sync.Mutex | |
| connected bool | |
| } | |
| // Send sends a message to the client | |
| func (c *Client) Send(message string) { | |
| c.mu.Lock() | |
| defer c.mu.Unlock() | |
| if !c.connected { | |
| return | |
| } | |
| select { | |
| case c.inChannel <- message: | |
| default: | |
| // Channel full, drop message to prevent blocking | |
| } | |
| } | |
| // Receive returns the next message for the client (blocking) | |
| func (c *Client) Receive() string { | |
| msg, ok := <-c.inChannel | |
| if !ok { | |
| return "" | |
| } | |
| return msg | |
| } |
| // Disconnect removes a client from the chat server | ||
| func (s *ChatServer) Disconnect(client *Client) { | ||
| // TODO: Implement this method | ||
| // Hint: remove from map, close channels | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
|
|
||
| delete(s.clients, client.username) | ||
| close(client.inChannel) | ||
| close(client.outChannel) | ||
|
|
||
| client.connected = false | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Prevent double-close panic and data races during disconnect.
If Disconnect is called twice for the same client, close(client.inChannel) will trigger a runtime panic. Additionally, to avoid data races with the Send method (which reads client.connected and writes to the channel), you must synchronize this block using the client's mutex.
🔒️ Proposed fix to synchronize and guard against panics
func (s *ChatServer) Disconnect(client *Client) {
- // TODO: Implement this method
- // Hint: remove from map, close channels
s.mu.Lock()
defer s.mu.Unlock()
+ client.mu.Lock()
+ defer client.mu.Unlock()
+
+ if !client.connected {
+ return
+ }
+
delete(s.clients, client.username)
close(client.inChannel)
- close(client.outChannel)
client.connected = false
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Disconnect removes a client from the chat server | |
| func (s *ChatServer) Disconnect(client *Client) { | |
| // TODO: Implement this method | |
| // Hint: remove from map, close channels | |
| s.mu.Lock() | |
| defer s.mu.Unlock() | |
| delete(s.clients, client.username) | |
| close(client.inChannel) | |
| close(client.outChannel) | |
| client.connected = false | |
| } | |
| // Disconnect removes a client from the chat server | |
| func (s *ChatServer) Disconnect(client *Client) { | |
| s.mu.Lock() | |
| defer s.mu.Unlock() | |
| client.mu.Lock() | |
| defer client.mu.Unlock() | |
| if !client.connected { | |
| return | |
| } | |
| delete(s.clients, client.username) | |
| close(client.inChannel) | |
| client.connected = false | |
| } |
| receiver.inChannel <- Message{ | ||
| Sender: sender.username, | ||
| Receiver: receiver.username, | ||
| Message: message, | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Use non-blocking Send to prevent server deadlocks.
Direct blocking channel sends while holding the server's RLock() can deadlock the entire server if a client's channel fills up (which blocks the send, thereby starving write locks like Disconnect). The shared root cause is bypassing the client's non-blocking Send method.
challenge-8/submissions/aruncs/solution-template.go#L104-L108: replace the blockingMessagesend with a call to the non-blockingreceiver.Send(message).challenge-8/submissions/aruncs/solution-template.go#L130-L134: replace the blockingMessagesend with a call to the non-blockingreceiver.Send(message).
📍 Affects 1 file
challenge-8/submissions/aruncs/solution-template.go#L104-L108(this comment)challenge-8/submissions/aruncs/solution-template.go#L130-L134
|
🎉 Auto-merged! This PR was automatically merged after 2 days with all checks passing. Thank you for your contribution, @aruncs! 📊 Scoreboards and badges will be updated shortly. |
1 participant
Challenge 8 Solution
Submitted by: @aruncs
Challenge: Challenge 8
Description
This PR contains my solution for Challenge 8.
Changes
challenge-8/submissions/aruncs/solution-template.goTesting
Thank you for reviewing my submission! 🚀