From bab2b1b9f6a0a2c0e0b243c16f814b5a73f74184 Mon Sep 17 00:00:00 2001 From: Sergey Matveev Date: Sat, 26 Mar 2016 17:10:04 +0300 Subject: [PATCH] Various race preventing locks --- client.go | 4 +-- daemon.go | 56 ++++++++++++++++++++++++++++++-- room.go | 92 +++++++++++++++++++++++++++++++++++++--------------- room_test.go | 10 +++++- 4 files changed, 130 insertions(+), 32 deletions(-) diff --git a/client.go b/client.go index e90d512..640a58a 100644 --- a/client.go +++ b/client.go @@ -51,7 +51,7 @@ type Client struct { sync.Mutex } -func (c Client) Host() string { +func (c *Client) Host() string { addr := c.conn.RemoteAddr().String() if host, _, err := net.SplitHostPort(addr); err == nil { addr = host @@ -62,7 +62,7 @@ func (c Client) Host() string { return addr } -func (c Client) String() string { +func (c *Client) String() string { return *c.nickname + "!" + *c.username + "@" + c.Host() } diff --git a/daemon.go b/daemon.go index b5c3e56..d46ee07 100644 --- a/daemon.go +++ b/daemon.go @@ -40,18 +40,23 @@ const ( var ( RENickname = regexp.MustCompile("^[a-zA-Z0-9-]{1,24}$") + clients map[*Client]struct{} = make(map[*Client]struct{}) + clientsM sync.RWMutex + rooms map[string]*Room = make(map[string]*Room) + roomsM sync.RWMutex roomsGroup sync.WaitGroup - - clients map[*Client]struct{} = make(map[*Client]struct{}) + roomSinks map[*Room]chan ClientEvent = make(map[*Room]chan ClientEvent) ) func SendLusers(client *Client) { lusers := 0 + clientsM.RLock() for client := range clients { if client.registered { lusers++ } } + clientsM.RUnlock() client.ReplyNicknamed("251", fmt.Sprintf("There are %d users and 0 invisible on 1 servers", lusers)) } @@ -82,11 +87,14 @@ func SendWhois(client *Client, nicknames []string) { var subscriber *Client for _, nickname := range nicknames { nickname = strings.ToLower(nickname) + clientsM.RLock() for c = range clients { if strings.ToLower(*c.nickname) == nickname { + clientsM.RUnlock() goto Found } } + clientsM.RUnlock() client.ReplyNoNickChan(nickname) continue Found: @@ -101,6 +109,7 @@ func SendWhois(client *Client, nicknames []string) { client.ReplyNicknamed("301", *c.nickname, *c.away) } subscriptions = make([]string, 0) + roomsM.RLock() for _, room = range rooms { for subscriber = range room.members { if *subscriber.nickname == nickname { @@ -108,6 +117,7 @@ func SendWhois(client *Client, nicknames []string) { } } } + roomsM.RUnlock() sort.Strings(subscriptions) client.ReplyNicknamed("319", *c.nickname, strings.Join(subscriptions, " ")) client.ReplyNicknamed("318", *c.nickname, "End of /WHOIS list") @@ -121,14 +131,17 @@ func SendList(client *Client, cols []string) { rs = strings.Split(strings.Split(cols[1], " ")[0], ",") } else { rs = make([]string, 0) + roomsM.RLock() for r = range rooms { rs = append(rs, r) } + roomsM.RUnlock() } sort.Strings(rs) var room *Room var found bool for _, r = range rs { + roomsM.RLock() if room, found = rooms[r]; found { client.ReplyNicknamed( "322", @@ -137,6 +150,7 @@ func SendList(client *Client, cols []string) { *room.topic, ) } + roomsM.RUnlock() } client.ReplyNicknamed("323", "End of /LIST") } @@ -163,12 +177,15 @@ func ClientRegister(client *Client, cmd string, cols []string) { // Compatibility with some clients prepending colons to nickname nickname = strings.TrimPrefix(nickname, ":") nickname = strings.ToLower(nickname) + clientsM.RLock() for existingClient := range clients { if *existingClient.nickname == nickname { + clientsM.RUnlock() client.ReplyParts("433", "*", nickname, "Nickname is already in use") return } } + clientsM.RUnlock() if !RENickname.MatchString(nickname) { client.ReplyParts("432", "*", cols[1], "Erroneous nickname") return @@ -227,8 +244,10 @@ func ClientRegister(client *Client, cmd string, cols []string) { func RoomRegister(name string) (*Room, chan ClientEvent) { roomNew := NewRoom(name) roomSink := make(chan ClientEvent) + roomsM.Lock() rooms[name] = roomNew roomSinks[roomNew] = roomSink + roomsM.Unlock() go roomNew.Processor(roomSink) roomsGroup.Add(1) return roomNew, roomSink @@ -257,8 +276,10 @@ func HandlerJoin(client *Client, cmd string) { } else { key = "" } + roomsM.RLock() for roomExisting, roomSink = range roomSinks { if room == *roomExisting.name { + roomsM.RUnlock() if (*roomExisting.key != "") && (*roomExisting.key != key) { goto Denied } @@ -266,6 +287,7 @@ func HandlerJoin(client *Client, cmd string) { goto Joined } } + roomsM.RUnlock() roomNew, roomSink = RoomRegister(room) log.Println("Room", roomNew, "created") if key != "" { @@ -293,6 +315,7 @@ func Processor(events chan ClientEvent, finished chan struct{}) { client := event.client switch event.eventType { case EventTick: + clientsM.RLock() for c := range clients { if c.recvTimestamp.Add(PingTimeout).Before(now) { log.Println(c, "ping timeout") @@ -309,6 +332,8 @@ func Processor(events chan ClientEvent, finished chan struct{}) { } } } + clientsM.RUnlock() + roomsM.Lock() for rn, r := range rooms { if *statedir == "" && len(r.members) == 0 { log.Println(rn, "emptied room") @@ -317,20 +342,29 @@ func Processor(events chan ClientEvent, finished chan struct{}) { delete(roomSinks, r) } } + roomsM.Unlock() case EventTerm: + roomsM.RLock() for _, sink := range roomSinks { sink <- ClientEvent{eventType: EventTerm} } + roomsM.RUnlock() roomsGroup.Wait() close(finished) return case EventNew: + clientsM.Lock() clients[client] = struct{}{} + clientsM.Unlock() case EventDel: + clientsM.Lock() delete(clients, client) + clientsM.Unlock() + roomsM.RLock() for _, roomSink := range roomSinks { roomSink <- event } + roomsM.RUnlock() case EventMsg: cols := strings.SplitN(event.text, " ", 2) cmd := strings.ToUpper(cols[0]) @@ -384,9 +418,11 @@ func Processor(events chan ClientEvent, finished chan struct{}) { continue } room := cols[0] + roomsM.RLock() r, found := rooms[room] if !found { client.ReplyNoChannel(room) + roomsM.RUnlock() continue } if len(cols) == 1 { @@ -394,6 +430,7 @@ func Processor(events chan ClientEvent, finished chan struct{}) { } else { roomSinks[r] <- ClientEvent{client, EventMode, cols[1]} } + roomsM.RUnlock() case "MOTD": SendMotd(client) case "PART": @@ -402,14 +439,17 @@ func Processor(events chan ClientEvent, finished chan struct{}) { continue } rs := strings.Split(cols[1], " ")[0] + roomsM.RLock() for _, room := range strings.Split(rs, ",") { if r, found := rooms[room]; found { roomSinks[r] <- ClientEvent{client, EventDel, ""} } else { + roomsM.RUnlock() client.ReplyNoChannel(room) continue } } + roomsM.RUnlock() case "PING": if len(cols) == 1 { client.ReplyNicknamed("409", "No origin specified") @@ -430,6 +470,7 @@ func Processor(events chan ClientEvent, finished chan struct{}) { } msg := "" target := strings.ToLower(cols[0]) + clientsM.RLock() for c := range clients { if *c.nickname == target { msg = fmt.Sprintf(":%s %s %s %s", client, cmd, *c.nickname, cols[1]) @@ -440,9 +481,11 @@ func Processor(events chan ClientEvent, finished chan struct{}) { break } } + clientsM.RUnlock() if msg != "" { continue } + roomsM.RLock() if r, found := rooms[target]; found { roomSinks[r] <- ClientEvent{ client, @@ -452,13 +495,16 @@ func Processor(events chan ClientEvent, finished chan struct{}) { } else { client.ReplyNoNickChan(target) } + roomsM.RUnlock() case "TOPIC": if len(cols) == 1 { client.ReplyNotEnoughParameters("TOPIC") continue } cols = strings.SplitN(cols[1], " ", 2) + roomsM.RLock() r, found := rooms[cols[0]] + roomsM.RUnlock() if !found { client.ReplyNoChannel(cols[0]) continue @@ -469,18 +515,22 @@ func Processor(events chan ClientEvent, finished chan struct{}) { } else { change = "" } + roomsM.RLock() roomSinks[r] <- ClientEvent{client, EventTopic, change} + roomsM.RUnlock() case "WHO": if len(cols) == 1 || len(cols[1]) < 1 { client.ReplyNotEnoughParameters("WHO") continue } room := strings.Split(cols[1], " ")[0] + roomsM.RLock() if r, found := rooms[room]; found { roomSinks[r] <- ClientEvent{client, EventWho, ""} } else { client.ReplyNoChannel(room) } + roomsM.RUnlock() case "WHOIS": if len(cols) == 1 || len(cols[1]) < 1 { client.ReplyNotEnoughParameters("WHOIS") @@ -495,9 +545,11 @@ func Processor(events chan ClientEvent, finished chan struct{}) { continue } nicksKnown := make(map[string]struct{}) + clientsM.RLock() for c := range clients { nicksKnown[*c.nickname] = struct{}{} } + clientsM.RUnlock() var nicksExists []string for _, nickname := range strings.Split(cols[1], " ") { if _, exists := nicksKnown[nickname]; exists { diff --git a/room.go b/room.go index dcad9d4..d5693e8 100644 --- a/room.go +++ b/room.go @@ -24,14 +24,11 @@ import ( "regexp" "sort" "strings" + "sync" ) var ( RERoom = regexp.MustCompile("^#[^\x00\x07\x0a\x0d ,:/]{1,200}$") - - rooms map[string]*Room = make(map[string]*Room) - - roomSinks map[*Room]chan ClientEvent = make(map[*Room]chan ClientEvent) ) // Sanitize room's name. It can consist of 1 to 50 ASCII symbols @@ -45,10 +42,14 @@ type Room struct { topic *string key *string members map[*Client]struct{} + sync.RWMutex } -func (room Room) String() string { - return *room.name +func (room *Room) String() (name string) { + room.RLock() + name = *room.name + room.RUnlock() + return } func NewRoom(name string) *Room { @@ -63,25 +64,31 @@ func NewRoom(name string) *Room { } func (room *Room) SendTopic(client *Client) { + room.RLock() if *room.topic == "" { - client.ReplyNicknamed("331", *room.name, "No topic is set") + client.ReplyNicknamed("331", room.String(), "No topic is set") } else { - client.ReplyNicknamed("332", *room.name, *room.topic) + client.ReplyNicknamed("332", room.String(), *room.topic) } + room.RUnlock() } // Send message to all room's subscribers, possibly excluding someone. func (room *Room) Broadcast(msg string, clientToIgnore ...*Client) { + room.RLock() for member := range room.members { if (len(clientToIgnore) > 0) && member == clientToIgnore[0] { continue } member.Msg(msg) } + room.RUnlock() } func (room *Room) StateSave() { - stateSink <- StateEvent{*room.name, *room.topic, *room.key} + room.RLock() + stateSink <- StateEvent{room.String(), *room.topic, *room.key} + room.RUnlock() } func (room *Room) Processor(events <-chan ClientEvent) { @@ -93,54 +100,74 @@ func (room *Room) Processor(events <-chan ClientEvent) { roomsGroup.Done() return case EventNew: + room.Lock() room.members[client] = struct{}{} if *verbose { log.Println(client, "joined", room.name) } + room.Unlock() room.SendTopic(client) - room.Broadcast(fmt.Sprintf(":%s JOIN %s", client, *room.name)) - logSink <- LogEvent{*room.name, *client.nickname, "joined", true} + room.Broadcast(fmt.Sprintf(":%s JOIN %s", client, room.String())) + logSink <- LogEvent{room.String(), *client.nickname, "joined", true} nicknames := make([]string, 0) + room.RLock() for member := range room.members { nicknames = append(nicknames, *member.nickname) } + room.RUnlock() sort.Strings(nicknames) - client.ReplyNicknamed("353", "=", *room.name, strings.Join(nicknames, " ")) - client.ReplyNicknamed("366", *room.name, "End of NAMES list") + client.ReplyNicknamed("353", "=", room.String(), strings.Join(nicknames, " ")) + client.ReplyNicknamed("366", room.String(), "End of NAMES list") case EventDel: + room.RLock() if _, subscribed := room.members[client]; !subscribed { - client.ReplyNicknamed("442", *room.name, "You are not on that channel") + client.ReplyNicknamed("442", room.String(), "You are not on that channel") + room.RUnlock() continue } + room.RUnlock() + room.Lock() delete(room.members, client) - msg := fmt.Sprintf(":%s PART %s :%s", client, *room.name, *client.nickname) + room.Unlock() + room.RLock() + msg := fmt.Sprintf(":%s PART %s :%s", client, room.String(), *client.nickname) room.Broadcast(msg) - logSink <- LogEvent{*room.name, *client.nickname, "left", true} + logSink <- LogEvent{room.String(), *client.nickname, "left", true} + room.RUnlock() case EventTopic: + room.RLock() if _, subscribed := room.members[client]; !subscribed { - client.ReplyParts("442", *room.name, "You are not on that channel") + client.ReplyParts("442", room.String(), "You are not on that channel") + room.RUnlock() continue } if event.text == "" { room.SendTopic(client) + room.RUnlock() continue } + room.RUnlock() topic := strings.TrimLeft(event.text, ":") + room.Lock() room.topic = &topic - msg := fmt.Sprintf(":%s TOPIC %s :%s", client, *room.name, *room.topic) + room.Unlock() + room.RLock() + msg := fmt.Sprintf(":%s TOPIC %s :%s", client, room.String(), *room.topic) room.Broadcast(msg) logSink <- LogEvent{ - *room.name, + room.String(), *client.nickname, "set topic to " + *room.topic, true, } + room.RUnlock() room.StateSave() case EventWho: + room.RLock() for m := range room.members { client.ReplyNicknamed( "352", - *room.name, + room.String(), *m.username, m.Host(), *hostname, @@ -149,29 +176,36 @@ func (room *Room) Processor(events <-chan ClientEvent) { "0 "+*m.realname, ) } - client.ReplyNicknamed("315", *room.name, "End of /WHO list") + client.ReplyNicknamed("315", room.String(), "End of /WHO list") + room.RUnlock() case EventMode: + room.RLock() if event.text == "" { mode := "+" if *room.key != "" { mode = mode + "k" } - client.Msg(fmt.Sprintf("324 %s %s %s", *client.nickname, *room.name, mode)) + client.Msg(fmt.Sprintf("324 %s %s %s", *client.nickname, room.String(), mode)) + room.RUnlock() continue } if strings.HasPrefix(event.text, "b") { - client.ReplyNicknamed("368", *room.name, "End of channel ban list") + client.ReplyNicknamed("368", room.String(), "End of channel ban list") + room.RUnlock() continue } if strings.HasPrefix(event.text, "-k") || strings.HasPrefix(event.text, "+k") { if _, subscribed := room.members[client]; !subscribed { - client.ReplyParts("442", *room.name, "You are not on that channel") + client.ReplyParts("442", room.String(), "You are not on that channel") + room.RUnlock() continue } } else { client.ReplyNicknamed("472", event.text, "Unknown MODE flag") + room.RUnlock() continue } + room.RUnlock() var msg string var msgLog string if strings.HasPrefix(event.text, "+k") { @@ -180,17 +214,21 @@ func (room *Room) Processor(events <-chan ClientEvent) { client.ReplyNotEnoughParameters("MODE") continue } + room.Lock() room.key = &cols[1] msg = fmt.Sprintf(":%s MODE %s +k %s", client, *room.name, *room.key) msgLog = "set channel key to " + *room.key + room.Unlock() } else { key := "" + room.Lock() room.key = &key msg = fmt.Sprintf(":%s MODE %s -k", client, *room.name) + room.Unlock() msgLog = "removed channel key" } room.Broadcast(msg) - logSink <- LogEvent{*room.name, *client.nickname, msgLog, true} + logSink <- LogEvent{room.String(), *client.nickname, msgLog, true} room.StateSave() case EventMsg: sep := strings.Index(event.text, " ") @@ -198,12 +236,12 @@ func (room *Room) Processor(events <-chan ClientEvent) { ":%s %s %s :%s", client, event.text[:sep], - *room.name, + room.String(), event.text[sep+1:]), client, ) logSink <- LogEvent{ - *room.name, + room.String(), *client.nickname, event.text[sep+1:], false, diff --git a/room_test.go b/room_test.go index 58f4158..8618fb9 100644 --- a/room_test.go +++ b/room_test.go @@ -47,9 +47,11 @@ func TestTwoUsers(t *testing.T) { host := "foohost" hostname = &host events := make(chan ClientEvent) + roomsM.Lock() rooms = make(map[string]*Room) - clients = make(map[*Client]struct{}) roomSinks = make(map[*Room]chan ClientEvent) + roomsM.Unlock() + clients = make(map[*Client]struct{}) finished := make(chan struct{}) go Processor(events, finished) defer func() { @@ -182,12 +184,14 @@ func TestJoin(t *testing.T) { for i := 0; i < 4*2; i++ { <-conn.outbound } + roomsM.RLock() if _, ok := rooms["#bar"]; !ok { t.Fatal("#bar does not exist") } if _, ok := rooms["#baz"]; !ok { t.Fatal("#baz does not exist") } + roomsM.RUnlock() if r := <-logSink; (r.what != "joined") || (r.where != "#bar") || (r.who != "nick2") || (r.meta != true) { t.Fatal("invalid join log event #bar", r) } @@ -199,12 +203,14 @@ func TestJoin(t *testing.T) { for i := 0; i < 4*2; i++ { <-conn.outbound } + roomsM.RLock() if *rooms["#barenc"].key != "key1" { t.Fatal("no room with key1") } if *rooms["#bazenc"].key != "key2" { t.Fatal("no room with key2") } + roomsM.RUnlock() if r := <-logSink; (r.what != "joined") || (r.where != "#barenc") || (r.who != "nick2") || (r.meta != true) { t.Fatal("invalid join log event #barenc", r) } @@ -222,9 +228,11 @@ func TestJoin(t *testing.T) { if r := <-conn.outbound; r != ":nick2!foo2@someclient MODE #barenc -k\r\n" { t.Fatal("remove #barenc key", r) } + roomsM.RLock() if *rooms["#barenc"].key != "" { t.Fatal("removing key from #barenc") } + roomsM.RUnlock() if r := <-logSink; (r.what != "removed channel key") || (r.where != "#barenc") || (r.who != "nick2") || (r.meta != true) { t.Fatal("removed channel key log", r) } -- 2.44.0