-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add solution for Challenge 8 by aruncs #1894
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,149 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Package challenge8 contains the solution for Challenge 8: Chat Server with Channels. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| package challenge8 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "errors" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "sync" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Add any other necessary imports | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type Message struct { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Sender string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Receiver string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Message string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // ChatServer manages client connections and message routing | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type ChatServer struct { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // TODO: Implement this struct | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Hint: clients map, mutex | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mu sync.RWMutex | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clients map[string]*Client | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // NewChatServer creates a new chat server instance | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func NewChatServer() *ChatServer { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // TODO: Implement this function | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return &ChatServer{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clients: make(map[string]*Client, 10), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Connect adds a new client to the chat server | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func (s *ChatServer) Connect(username string) (*Client, error) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // TODO: Implement this method | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Hint: check username, create client, add to map | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| s.mu.Lock() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| defer s.mu.Unlock() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if s.isUsernameExists(username) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil, ErrUsernameAlreadyTaken | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| client := &Client{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| username: username, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| inChannel: make(chan Message, 10), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| outChannel: make(chan Message, 10), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| connected: true, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| s.clients[username] = client | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return client, nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+79
to
+91
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win Prevent double-close panic and data races during disconnect. If 🔒️ 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Broadcast sends a message to all connected clients | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func (s *ChatServer) Broadcast(sender *Client, message string) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // TODO: Implement this method | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Hint: format message, send to all clients | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| s.mu.RLock() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| defer s.mu.RUnlock() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for _, receiver := range s.clients { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if receiver.username == sender.username { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| receiver.inChannel <- Message{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Sender: sender.username, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Receiver: receiver.username, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Message: message, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+104
to
+108
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win Use non-blocking Direct blocking channel sends while holding the server's
📍 Affects 1 file
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // PrivateMessage sends a message to a specific client | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func (s *ChatServer) PrivateMessage(sender *Client, recipient string, message string) error { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // TODO: Implement this method | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Hint: find recipient, check errors, send message | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| s.mu.RLock() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| defer s.mu.RUnlock() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if !sender.connected { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return ErrClientDisconnected | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| receiver, exists := s.clients[recipient] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if !exists { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return ErrRecipientNotFound | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| receiver.inChannel <- Message{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Sender: sender.username, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Receiver: recipient, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Message: message, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func (s *ChatServer) isUsernameExists(username string) bool { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _, exists := s.clients[username] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return exists | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Common errors that can be returned by the Chat Server | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ErrUsernameAlreadyTaken = errors.New("username already taken") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ErrRecipientNotFound = errors.New("recipient not found") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ErrClientDisconnected = errors.New("client disconnected") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Add more error types as needed | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Implement thread-safe
Sendand fixReceivechannel handling.There are several critical functional and stability issues here:
Sendis entirely unwritten, meaning messages are never dispatched through the client's public API.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-blockingSendmethod.🛠️ 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