Challenge 8 – Chat Server with Channels (Go) #869
Conversation
WalkthroughThe PR adds two Go solutions for coding challenges: a simple program computing the sum of two integers for challenge-1, and a concurrent chat server with client connection management, broadcast messaging, and private messaging for challenge-8. Changes
Sequence DiagramsequenceDiagram
actor User1 as User 1
participant CS as ChatServer
actor User2 as User 2
User1->>CS: Connect(username="alice")
CS->>CS: Validate username uniqueness
CS-->>User1: Return Client & Outgoing channel
User2->>CS: Connect(username="bob")
CS->>CS: Validate username uniqueness
CS-->>User2: Return Client & Outgoing channel
User1->>CS: Broadcast("hello everyone")
CS->>CS: Format "alice: hello everyone"
CS->>User2: Send to User2's Outgoing channel
CS-->>User1: Log sent to 1 client
User2->>CS: PrivateMessage(recipient="alice", message="hi alice")
CS->>CS: Validate sender connected
CS->>CS: Locate recipient "alice"
CS->>User1: Send "bob: hi alice"
CS-->>User2: Return success
User1->>CS: Disconnect(client)
CS->>CS: Remove from clients map
CS->>CS: Close Outgoing channel
CS-->>User1: Logged disconnection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring extra attention:
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 2
🧹 Nitpick comments (2)
challenge-8/submissions/brenoamin/solution-template.go (2)
8-8: Unused import:netpackage is imported but never used.The
Connfield is always set tonilinConnect. Consider removing the import and theConnfield if network connections are not being used.import ( "errors" "fmt" "log" - "net" "strings" "sync" - // Add any other necessary imports )And in the
Clientstruct:type Client struct { Username string - Conn net.Conn Outgoing chan string Disconnected bool mu sync.Mutex }
48-56: Consider removing unused channel fields.The
broadcast,join, andleavechannels are declared but never used. The current implementation routes messages via direct method calls rather than a channel-based event loop. Removing these fields would reduce confusion.type ChatServer struct { - // Hint: clients map, mutex - clients map[*Client]bool - broadcast chan string - join chan *Client - leave chan *Client - mu sync.Mutex + clients map[*Client]bool + mu sync.Mutex }And update
NewChatServer:func NewChatServer() *ChatServer { return &ChatServer{ - clients: make(map[*Client]bool), - broadcast: make(chan string), + clients: make(map[*Client]bool), } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
challenge-1/submissions/brenoamin/solution-template.go(1 hunks)challenge-8/submissions/brenoamin/solution-template.go(1 hunks)
🔇 Additional comments (5)
challenge-1/submissions/brenoamin/solution-template.go (1)
1-24: LGTM!The implementation is clean and straightforward. Error handling for input parsing is properly implemented, and the
Sumfunction correctly returns the sum of two integers.challenge-8/submissions/brenoamin/solution-template.go (4)
66-90: LGTM!The
Connectmethod correctly usesdefer s.mu.Unlock()to ensure the mutex is always released, and properly validates username uniqueness before adding the client.
92-111: LGTM!The
Disconnectmethod correctly releases the server lock before acquiring the client lock, avoiding potential lock ordering issues. The check for!client.Disconnectedbefore closing the channel prevents panics from double-close.
182-189: LGTM!The error variables are properly defined and exported, following Go conventions.
24-37: LGTM with a minor note.The non-blocking send pattern with
select/defaultprevents deadlocks when the channel buffer is full, but silently drops messages. This is an acceptable trade-off for a chat server, though you may want to add a log statement in thedefaultcase for debugging purposes.
| sender.mu.Lock() | ||
| if sender.Disconnected { | ||
| log.Printf("[broadcast-ignored] sender=%s reason=disconnected", sender.Username) | ||
| return | ||
| } | ||
| sender.mu.Unlock() |
There was a problem hiding this comment.
Critical: Mutex deadlock on early return.
When sender.Disconnected is true, the function returns at line 124 without releasing the lock acquired at line 121. This will cause a deadlock on any subsequent operation involving sender.
sender.mu.Lock()
if sender.Disconnected {
log.Printf("[broadcast-ignored] sender=%s reason=disconnected", sender.Username)
+ sender.mu.Unlock()
return
}
sender.mu.Unlock()📝 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.
| sender.mu.Lock() | |
| if sender.Disconnected { | |
| log.Printf("[broadcast-ignored] sender=%s reason=disconnected", sender.Username) | |
| return | |
| } | |
| sender.mu.Unlock() | |
| sender.mu.Lock() | |
| if sender.Disconnected { | |
| log.Printf("[broadcast-ignored] sender=%s reason=disconnected", sender.Username) | |
| sender.mu.Unlock() | |
| return | |
| } | |
| sender.mu.Unlock() |
🤖 Prompt for AI Agents
In challenge-8/submissions/brenoamin/solution-template.go around lines 121 to
126, the code acquires sender.mu.Lock() then returns early when
sender.Disconnected is true, causing a mutex deadlock; fix by ensuring the mutex
is always released before any return — either check sender.Disconnected without
holding the lock (move the check before acquiring the mutex), or immediately
defer sender.mu.Unlock() right after locking so the lock is released on all
return paths, and remove the premature return while locked.
| sender.mu.Lock() | ||
| if sender.Disconnected { | ||
| return ErrSenderDisconnected | ||
| } | ||
| sender.mu.Unlock() |
There was a problem hiding this comment.
Critical: Mutex deadlock on early return.
Same issue as in Broadcast: when sender.Disconnected is true, the function returns at line 151 without releasing the lock acquired at line 149.
sender.mu.Lock()
if sender.Disconnected {
+ sender.mu.Unlock()
return ErrSenderDisconnected
}
sender.mu.Unlock()📝 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.
| sender.mu.Lock() | |
| if sender.Disconnected { | |
| return ErrSenderDisconnected | |
| } | |
| sender.mu.Unlock() | |
| sender.mu.Lock() | |
| if sender.Disconnected { | |
| sender.mu.Unlock() | |
| return ErrSenderDisconnected | |
| } | |
| sender.mu.Unlock() |
🤖 Prompt for AI Agents
In challenge-8/submissions/brenoamin/solution-template.go around lines 149 to
153, the code acquires sender.mu.Lock() then returns early when
sender.Disconnected is true, causing a mutex deadlock; fix by ensuring the lock
is always released before any return — either unlock explicitly before returning
or use defer sender.mu.Unlock() immediately after acquiring the lock so the
mutex is released on all control paths.
1 participant
Overview
This project implements a concurrent chat server in Go using goroutines and channels.
It supports multiple clients, message broadcasting, private messaging, and safe connection
management, fully aligned with the challenge requirements.
Features
Architecture
Concurrency Model
Testing
The implementation is validated with tests covering: