From 52b0fb71e78d918175e34c740b7e20d83fbe92a1 Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Wed, 22 Nov 2017 04:41:11 -0500 Subject: [PATCH 1/3] refactor ClientManager --- DEVELOPING.md | 19 +++++ README.md | 2 +- irc/channel.go | 2 +- irc/client.go | 39 ++------- irc/client_lookup_set.go | 171 +++++++++++++++++---------------------- irc/dline.go | 6 +- irc/idletimer.go | 2 +- irc/kline.go | 6 +- irc/monitor.go | 2 +- irc/nickname.go | 72 +++++++---------- irc/server.go | 27 +++---- irc/snomanager.go | 2 +- irc/whowas.go | 2 +- 13 files changed, 146 insertions(+), 206 deletions(-) diff --git a/DEVELOPING.md b/DEVELOPING.md index 68da5afa..2587fdb9 100644 --- a/DEVELOPING.md +++ b/DEVELOPING.md @@ -80,3 +80,22 @@ To debug a hang, the best thing to do is to get a stack trace. Go's nice, and yo $ kill -ABRT This will kill Oragono and print out a stack trace for you to take a look at. + +## Concurrency design + +Oragono involves a fair amount of shared state. Here are some of the main points: + +1. Each client has a separate goroutine that listens for incoming messages and synchronously processes them. +1. All sends to clients are asynchronous; `client.Send` appends the message to a queue, which is then processed on a separate goroutine. It is always safe to call `client.Send`. +1. The server has a few of its own goroutines, for listening on sockets and handing off new client connections to their dedicated goroutines. +1. A few tasks are done asynchronously in ad-hoc goroutines. + +In consequence, there is a lot of state (in particular, server and channel state) that can be read and written from multiple goroutines. This state is protected with mutexes. To avoid deadlocks, mutexes are arranged in "tiers"; while holding a mutex of one tier, you're only allowed to acquire mutexes of a strictly *higher* tier. The tiers are: + +1. Tier 1 mutexes: these are the "innermost" mutexes. They typically protect getters and setters on objects, or invariants that are local to the state of a single object. Example: `Channel.stateMutex`. +1. Tier 2 mutexes: these protect some invariants of their own, but also need to access fields on other objects that themselves require synchronization. Example: `ChannelManager.RWMutex`. +1. Tier 3 mutexes: these protect macroscopic operations, where it doesn't make sense for more than one to occur concurrently. Example; `Server.rehashMutex`, which prevents rehashes from overlapping. + +There are some mutexes that are "tier 0": anything in a subpackage of `irc` (e.g., `irc/logger` or `irc/connection_limits`) shouldn't acquire mutexes defined in `irc`. + +We are using `buntdb` for persistence; a `buntdb.DB` has an `RWMutex` inside it, with read-write transactions getting the `Lock()` and read-only transactions getting the `RLock()`. We haven't completely decided where this lock fits into the overall lock model. For now, it's probably better to err on the side of caution: if possible, don't acquire new locks inside the `buntdb` transaction, and be careful about what locks are held around the transaction as well. diff --git a/README.md b/README.md index e9fb961b..05c5270b 100644 --- a/README.md +++ b/README.md @@ -68,7 +68,7 @@ The `stable` branch contains the latest release. You can run this for a producti [![Build Status](https://travis-ci.org/oragono/oragono.svg?branch=master)](https://travis-ci.org/oragono/oragono) -Clone the appropriate branch. From the root folder, run `make` to generate all release files for all of our target OSes: +Clone the appropriate branch. If necessary, do `git submodule update --init` to set up vendored dependencies. From the root folder, run `make` to generate all release files for all of our target OSes: ``` make ``` diff --git a/irc/channel.go b/irc/channel.go index 10af4d46..6c4c77f8 100644 --- a/irc/channel.go +++ b/irc/channel.go @@ -35,7 +35,7 @@ type Channel struct { createdTime time.Time registeredFounder string registeredTime time.Time - stateMutex sync.RWMutex + stateMutex sync.RWMutex // tier 1 topic string topicSetBy string topicSetTime time.Time diff --git a/irc/client.go b/irc/client.go index 71c95a1b..6deb4410 100644 --- a/irc/client.go +++ b/irc/client.go @@ -73,7 +73,7 @@ type Client struct { saslValue string server *Server socket *Socket - stateMutex sync.RWMutex // generic protection for mutable state + stateMutex sync.RWMutex // tier 1 username string vhost string whoisLine string @@ -313,11 +313,15 @@ func (client *Client) IdleSeconds() uint64 { // HasNick returns true if the client's nickname is set (used in registration). func (client *Client) HasNick() bool { + client.stateMutex.RLock() + defer client.stateMutex.RUnlock() return client.nick != "" && client.nick != "*" } // HasUsername returns true if the client's username is set (used in registration). func (client *Client) HasUsername() bool { + client.stateMutex.RLock() + defer client.stateMutex.RUnlock() return client.username != "" && client.username != "*" } @@ -403,6 +407,7 @@ func (client *Client) updateNickMask(nick string) { } client.stateMutex.Lock() + defer client.stateMutex.Unlock() if len(client.vhost) > 0 { client.hostname = client.vhost @@ -419,8 +424,6 @@ func (client *Client) updateNickMask(nick string) { client.nickMaskString = nickMaskString client.nickMaskCasefolded = nickMaskCasefolded - - client.stateMutex.Unlock() } // AllNickmasks returns all the possible nickmasks for the client. @@ -449,36 +452,6 @@ func (client *Client) AllNickmasks() []string { return masks } -// SetNickname sets the very first nickname for the client. -func (client *Client) SetNickname(nickname string) error { - if client.HasNick() { - client.server.logger.Error("nick", fmt.Sprintf("%s nickname already set, something is wrong with server consistency", client.nickMaskString)) - return ErrNickAlreadySet - } - - err := client.server.clients.Add(client, nickname) - if err == nil { - client.updateNick(nickname) - } - return err -} - -// ChangeNickname changes the existing nickname of the client. -func (client *Client) ChangeNickname(nickname string) error { - origNickMask := client.nickMaskString - err := client.server.clients.Replace(client.nick, nickname, client) - if err == nil { - client.server.logger.Debug("nick", fmt.Sprintf("%s changed nickname to %s", client.nick, nickname)) - client.server.snomasks.Send(sno.LocalNicks, fmt.Sprintf(ircfmt.Unescape("$%s$r changed nickname to %s"), client.nick, nickname)) - client.server.whoWas.Append(client) - client.updateNickMask(nickname) - for friend := range client.Friends() { - friend.Send(nil, origNickMask, "NICK", nickname) - } - } - return err -} - // LoggedIntoAccount returns true if this client is logged into an account. func (client *Client) LoggedIntoAccount() bool { return client.account != nil && client.account != &NoAccount diff --git a/irc/client_lookup_set.go b/irc/client_lookup_set.go index bf9a6097..0dabe6ca 100644 --- a/irc/client_lookup_set.go +++ b/irc/client_lookup_set.go @@ -18,9 +18,8 @@ import ( ) var ( - ErrNickMissing = errors.New("nick missing") - ErrNicknameInUse = errors.New("nickname in use") - ErrNicknameMismatch = errors.New("nickname mismatch") + ErrNickMissing = errors.New("nick missing") + ErrNicknameInUse = errors.New("nickname in use") ) // ExpandUserHost takes a userhost, and returns an expanded version. @@ -37,132 +36,108 @@ func ExpandUserHost(userhost string) (expanded string) { return } -// ClientLookupSet represents a way to store, search and lookup clients. -type ClientLookupSet struct { - ByNickMutex sync.RWMutex - ByNick map[string]*Client +// ClientManager keeps track of clients by nick, enforcing uniqueness of casefolded nicks +type ClientManager struct { + sync.RWMutex // tier 2 + byNick map[string]*Client } -// NewClientLookupSet returns a new lookup set. -func NewClientLookupSet() *ClientLookupSet { - return &ClientLookupSet{ - ByNick: make(map[string]*Client), +// NewClientManager returns a new ClientManager. +func NewClientManager() *ClientManager { + return &ClientManager{ + byNick: make(map[string]*Client), } } -// Count returns how many clients are in the lookup set. -func (clients *ClientLookupSet) Count() int { - clients.ByNickMutex.RLock() - defer clients.ByNickMutex.RUnlock() - count := len(clients.ByNick) +// Count returns how many clients are in the manager. +func (clients *ClientManager) Count() int { + clients.RLock() + defer clients.RUnlock() + count := len(clients.byNick) return count } -// Has returns whether or not the given client exists. -//TODO(dan): This seems like ripe ground for a race, if code does Has then Get, and assumes the Get will return a client. -func (clients *ClientLookupSet) Has(nick string) bool { +// Get retrieves a client from the manager, if they exist. +func (clients *ClientManager) Get(nick string) *Client { casefoldedName, err := CasefoldName(nick) if err == nil { - return false - } - clients.ByNickMutex.RLock() - defer clients.ByNickMutex.RUnlock() - _, exists := clients.ByNick[casefoldedName] - return exists -} - -// getNoMutex is used internally, for getting clients when no mutex is required (i.e. is already set). -func (clients *ClientLookupSet) getNoMutex(nick string) *Client { - casefoldedName, err := CasefoldName(nick) - if err == nil { - cli := clients.ByNick[casefoldedName] + clients.RLock() + defer clients.RUnlock() + cli := clients.byNick[casefoldedName] return cli } return nil } -// Get retrieves a client from the set, if they exist. -func (clients *ClientLookupSet) Get(nick string) *Client { - casefoldedName, err := CasefoldName(nick) - if err == nil { - clients.ByNickMutex.RLock() - defer clients.ByNickMutex.RUnlock() - cli := clients.ByNick[casefoldedName] - return cli +func (clients *ClientManager) removeInternal(client *Client) (removed bool) { + // requires holding ByNickMutex + oldcfnick := client.NickCasefolded() + currentEntry, present := clients.byNick[oldcfnick] + if present { + if currentEntry == client { + delete(clients.byNick, oldcfnick) + removed = true + } else { + // this shouldn't happen, but we can ignore it + client.server.logger.Warning("internal", fmt.Sprintf("clients for nick %s out of sync", oldcfnick)) + } } - return nil -} - -// Add adds a client to the lookup set. -func (clients *ClientLookupSet) Add(client *Client, nick string) error { - nick, err := CasefoldName(nick) - if err != nil { - return err - } - clients.ByNickMutex.Lock() - defer clients.ByNickMutex.Unlock() - if clients.getNoMutex(nick) != nil { - return ErrNicknameInUse - } - clients.ByNick[nick] = client - return nil + return } // Remove removes a client from the lookup set. -func (clients *ClientLookupSet) Remove(client *Client) error { +func (clients *ClientManager) Remove(client *Client) error { + clients.Lock() + defer clients.Unlock() + if !client.HasNick() { return ErrNickMissing } - clients.ByNickMutex.Lock() - defer clients.ByNickMutex.Unlock() - if clients.getNoMutex(client.nick) != client { - return ErrNicknameMismatch - } - delete(clients.ByNick, client.nickCasefolded) + clients.removeInternal(client) return nil } -// Replace renames an existing client in the lookup set. -func (clients *ClientLookupSet) Replace(oldNick, newNick string, client *Client) error { - // get casefolded nicknames - oldNick, err := CasefoldName(oldNick) - if err != nil { - return err - } - newNick, err = CasefoldName(newNick) +// SetNick sets a client's nickname, validating it against nicknames in use +func (clients *ClientManager) SetNick(client *Client, newNick string) error { + newcfnick, err := CasefoldName(newNick) if err != nil { return err } - // remove and replace - clients.ByNickMutex.Lock() - defer clients.ByNickMutex.Unlock() + clients.Lock() + defer clients.Unlock() - oldClient := clients.ByNick[newNick] - if oldClient == nil || oldClient == client { - // whoo - } else { + clients.removeInternal(client) + currentNewEntry := clients.byNick[newcfnick] + // the client may just be changing case + if currentNewEntry != nil && currentNewEntry != client { return ErrNicknameInUse } - - if oldNick == newNick { - // if they're only changing case, don't need to remove+re-add them - return nil - } - - delete(clients.ByNick, oldNick) - clients.ByNick[newNick] = client + clients.byNick[newcfnick] = client + client.updateNickMask(newNick) return nil } +func (clients *ClientManager) AllClients() (result []*Client) { + clients.RLock() + defer clients.RUnlock() + result = make([]*Client, len(clients.byNick)) + i := 0 + for _, client := range(clients.byNick) { + result[i] = client + i++ + } + return +} + // AllWithCaps returns all clients with the given capabilities. -func (clients *ClientLookupSet) AllWithCaps(capabs ...caps.Capability) (set ClientSet) { +func (clients *ClientManager) AllWithCaps(capabs ...caps.Capability) (set ClientSet) { set = make(ClientSet) - clients.ByNickMutex.RLock() - defer clients.ByNickMutex.RUnlock() + clients.RLock() + defer clients.RUnlock() var client *Client - for _, client = range clients.ByNick { + for _, client = range clients.byNick { // make sure they have all the required caps for _, capab := range capabs { if !client.capabilities.Has(capab) { @@ -177,7 +152,7 @@ func (clients *ClientLookupSet) AllWithCaps(capabs ...caps.Capability) (set Clie } // FindAll returns all clients that match the given userhost mask. -func (clients *ClientLookupSet) FindAll(userhost string) (set ClientSet) { +func (clients *ClientManager) FindAll(userhost string) (set ClientSet) { set = make(ClientSet) userhost, err := Casefold(ExpandUserHost(userhost)) @@ -186,9 +161,9 @@ func (clients *ClientLookupSet) FindAll(userhost string) (set ClientSet) { } matcher := ircmatch.MakeMatch(userhost) - clients.ByNickMutex.RLock() - defer clients.ByNickMutex.RUnlock() - for _, client := range clients.ByNick { + clients.RLock() + defer clients.RUnlock() + for _, client := range clients.byNick { if matcher.Match(client.nickMaskCasefolded) { set.Add(client) } @@ -198,7 +173,7 @@ func (clients *ClientLookupSet) FindAll(userhost string) (set ClientSet) { } // Find returns the first client that matches the given userhost mask. -func (clients *ClientLookupSet) Find(userhost string) *Client { +func (clients *ClientManager) Find(userhost string) *Client { userhost, err := Casefold(ExpandUserHost(userhost)) if err != nil { return nil @@ -206,9 +181,9 @@ func (clients *ClientLookupSet) Find(userhost string) *Client { matcher := ircmatch.MakeMatch(userhost) var matchedClient *Client - clients.ByNickMutex.RLock() - defer clients.ByNickMutex.RUnlock() - for _, client := range clients.ByNick { + clients.RLock() + defer clients.RUnlock() + for _, client := range clients.byNick { if matcher.Match(client.nickMaskCasefolded) { matchedClient = client break diff --git a/irc/dline.go b/irc/dline.go index d559813e..ccf54164 100644 --- a/irc/dline.go +++ b/irc/dline.go @@ -82,7 +82,7 @@ type dLineNet struct { // DLineManager manages and dlines. type DLineManager struct { - sync.RWMutex + sync.RWMutex // tier 1 // addresses that are dlined addresses map[string]*dLineAddr // networks that are dlined @@ -386,8 +386,7 @@ func dlineHandler(server *Server, client *Client, msg ircmsg.IrcMessage) bool { var killedClientNicks []string var toKill bool - server.clients.ByNickMutex.RLock() - for _, mcl := range server.clients.ByNick { + for _, mcl := range server.clients.AllClients() { if hostNet == nil { toKill = hostAddr.Equal(mcl.IP()) } else { @@ -399,7 +398,6 @@ func dlineHandler(server *Server, client *Client, msg ircmsg.IrcMessage) bool { killedClientNicks = append(killedClientNicks, mcl.nick) } } - server.clients.ByNickMutex.RUnlock() for _, mcl := range clientsToKill { mcl.exitedSnomaskSent = true diff --git a/irc/idletimer.go b/irc/idletimer.go index 07f624f9..209f1a96 100644 --- a/irc/idletimer.go +++ b/irc/idletimer.go @@ -29,7 +29,7 @@ const ( ) type IdleTimer struct { - sync.Mutex + sync.Mutex // tier 1 // immutable after construction registerTimeout time.Duration diff --git a/irc/kline.go b/irc/kline.go index 3a38a3d9..8ef05364 100644 --- a/irc/kline.go +++ b/irc/kline.go @@ -35,7 +35,7 @@ type KLineInfo struct { // KLineManager manages and klines. type KLineManager struct { - sync.RWMutex + sync.RWMutex // tier 1 // kline'd entries entries map[string]*KLineInfo } @@ -282,8 +282,7 @@ func klineHandler(server *Server, client *Client, msg ircmsg.IrcMessage) bool { var clientsToKill []*Client var killedClientNicks []string - server.clients.ByNickMutex.RLock() - for _, mcl := range server.clients.ByNick { + for _, mcl := range server.clients.AllClients() { for _, clientMask := range mcl.AllNickmasks() { if matcher.Match(clientMask) { clientsToKill = append(clientsToKill, mcl) @@ -291,7 +290,6 @@ func klineHandler(server *Server, client *Client, msg ircmsg.IrcMessage) bool { } } } - server.clients.ByNickMutex.RUnlock() for _, mcl := range clientsToKill { mcl.exitedSnomaskSent = true diff --git a/irc/monitor.go b/irc/monitor.go index 27d8780f..54d2400f 100644 --- a/irc/monitor.go +++ b/irc/monitor.go @@ -15,7 +15,7 @@ import ( // MonitorManager keeps track of who's monitoring which nicks. type MonitorManager struct { - sync.RWMutex + sync.RWMutex // tier 2 // client -> nicks it's watching watching map[*Client]map[string]bool // nick -> clients watching it diff --git a/irc/nickname.go b/irc/nickname.go index c0db39b6..0599e021 100644 --- a/irc/nickname.go +++ b/irc/nickname.go @@ -8,7 +8,9 @@ import ( "fmt" "strings" + "github.com/goshuirc/irc-go/ircfmt" "github.com/goshuirc/irc-go/ircmsg" + "github.com/oragono/oragono/irc/sno" ) var ( @@ -26,7 +28,11 @@ func nickHandler(server *Server, client *Client, msg ircmsg.IrcMessage) bool { return true } - nicknameRaw := strings.TrimSpace(msg.Params[0]) + return performNickChange(server, client, client, msg.Params[0]) +} + +func performNickChange(server *Server, client *Client, target *Client, newnick string) bool { + nicknameRaw := strings.TrimSpace(newnick) nickname, err := CasefoldName(nicknameRaw) if len(nicknameRaw) < 1 { @@ -39,16 +45,14 @@ func nickHandler(server *Server, client *Client, msg ircmsg.IrcMessage) bool { return false } - if client.nick == nickname { + if target.Nick() == nicknameRaw { return false } - // bleh, this will be replaced and done below - if client.registered { - err = client.ChangeNickname(nicknameRaw) - } else { - err = client.SetNickname(nicknameRaw) - } + hadNick := target.HasNick() + origNick := target.Nick() + origNickMask := target.NickMaskString() + err = client.server.clients.SetNick(target, nickname) if err == ErrNicknameInUse { client.Send(nil, server.name, ERR_NICKNAMEINUSE, client.nick, nicknameRaw, "Nickname is already in use") return false @@ -56,49 +60,31 @@ func nickHandler(server *Server, client *Client, msg ircmsg.IrcMessage) bool { client.Send(nil, server.name, ERR_UNKNOWNERROR, client.nick, "NICK", fmt.Sprintf("Could not set or change nickname: %s", err.Error())) return false } - if client.registered { - client.server.monitorManager.AlertAbout(client, true) + + client.server.logger.Debug("nick", fmt.Sprintf("%s changed nickname to %s", origNickMask, nickname)) + if hadNick { + target.server.snomasks.Send(sno.LocalNicks, fmt.Sprintf(ircfmt.Unescape("$%s$r changed nickname to %s"), origNick, nicknameRaw)) + target.server.whoWas.Append(client) + for friend := range target.Friends() { + friend.Send(nil, origNickMask, "NICK", nicknameRaw) + } + } + + if target.registered { + client.server.monitorManager.AlertAbout(target, true) + } else { + server.tryRegister(target) } - server.tryRegister(client) return false } // SANICK func sanickHandler(server *Server, client *Client, msg ircmsg.IrcMessage) bool { - if !client.authorized { - client.Quit("Bad password") - return true - } - - oldnick, oerr := CasefoldName(msg.Params[0]) - nickname, err := CasefoldName(msg.Params[1]) - - if len(nickname) < 1 { - client.Send(nil, server.name, ERR_NONICKNAMEGIVEN, client.nick, "No nickname given") - return false - } - - if oerr != nil || err != nil || len(strings.TrimSpace(msg.Params[1])) > server.limits.NickLen || restrictedNicknames[nickname] { - client.Send(nil, server.name, ERR_ERRONEUSNICKNAME, client.nick, msg.Params[0], "Erroneous nickname") - return false - } - - if client.nick == msg.Params[1] { - return false - } - - target := server.clients.Get(oldnick) + targetNick := strings.TrimSpace(msg.Params[0]) + target := server.clients.Get(targetNick) if target == nil { client.Send(nil, server.name, ERR_NOSUCHNICK, client.nick, msg.Params[0], "No such nick") return false } - - //TODO(dan): There's probably some races here, we should be changing this in the primary server thread - if server.clients.Get(nickname) != nil && server.clients.Get(nickname) != target { - client.Send(nil, server.name, ERR_NICKNAMEINUSE, client.nick, msg.Params[0], "Nickname is already in use") - return false - } - - target.ChangeNickname(msg.Params[1]) - return false + return performNickChange(server, client, target, msg.Params[1]) } diff --git a/irc/server.go b/irc/server.go index 4714401f..1c7216eb 100644 --- a/irc/server.go +++ b/irc/server.go @@ -74,7 +74,7 @@ type ListenerWrapper struct { // lets the ListenerWrapper inform the server that it has stopped: stopEvent chan bool // protects atomic update of tlsConfig and shouldStop: - configMutex sync.Mutex + configMutex sync.Mutex // tier 1 } // Server is the main Oragono server. @@ -86,10 +86,10 @@ type Server struct { channels *ChannelManager channelRegistry *ChannelRegistry checkIdent bool - clients *ClientLookupSet + clients *ClientManager commands chan Command configFilename string - configurableStateMutex sync.RWMutex // generic protection for server state modified by rehash() + configurableStateMutex sync.RWMutex // tier 1; generic protection for server state modified by rehash() connectionLimiter *connection_limits.Limiter connectionThrottler *connection_limits.Throttler ctime time.Time @@ -113,7 +113,7 @@ type Server struct { password []byte passwords *passwd.SaltedManager recoverFromErrors bool - rehashMutex sync.Mutex + rehashMutex sync.Mutex // tier 3 rehashSignal chan os.Signal proxyAllowedFrom []string signals chan os.Signal @@ -149,7 +149,7 @@ func NewServer(config *Config, logger *logger.Manager) (*Server, error) { server := &Server{ accounts: make(map[string]*ClientAccount), channels: NewChannelManager(), - clients: NewClientLookupSet(), + clients: NewClientManager(), commands: make(chan Command), connectionLimiter: connection_limits.NewLimiter(), connectionThrottler: connection_limits.NewThrottler(), @@ -238,11 +238,9 @@ func loadChannelList(channel *Channel, list string, maskMode Mode) { // Shutdown shuts down the server. func (server *Server) Shutdown() { //TODO(dan): Make sure we disallow new nicks - server.clients.ByNickMutex.RLock() - for _, client := range server.clients.ByNick { + for _, client := range server.clients.AllClients() { client.Notice("Server is shutting down") } - server.clients.ByNickMutex.RUnlock() if err := server.store.Close(); err != nil { server.logger.Error("shutdown", fmt.Sprintln("Could not close datastore:", err)) @@ -1332,11 +1330,9 @@ func (server *Server) applyConfig(config *Config, initial bool) error { server.configurableStateMutex.Unlock() // update on all clients - server.clients.ByNickMutex.RLock() - for _, sClient := range server.clients.ByNick { + for _, sClient := range server.clients.AllClients() { sClient.socket.MaxSendQBytes = config.Server.MaxSendQBytes } - server.clients.ByNickMutex.RUnlock() } // set RPL_ISUPPORT @@ -1370,8 +1366,7 @@ func (server *Server) applyConfig(config *Config, initial bool) error { if !initial { // push new info to all of our clients - server.clients.ByNickMutex.RLock() - for _, sClient := range server.clients.ByNick { + for _, sClient := range server.clients.AllClients() { for _, tokenline := range newISupportReplies { sClient.Send(nil, server.name, RPL_ISUPPORT, append([]string{sClient.nick}, tokenline...)...) } @@ -1380,7 +1375,6 @@ func (server *Server) applyConfig(config *Config, initial bool) error { sClient.Notice(rawIONotice) } } - server.clients.ByNickMutex.RUnlock() } return nil @@ -2008,10 +2002,7 @@ func lusersHandler(server *Server, client *Client, msg ircmsg.IrcMessage) bool { //TODO(vegax87) Fix network statistics and additional parameters var totalcount, invisiblecount, opercount int - server.clients.ByNickMutex.RLock() - defer server.clients.ByNickMutex.RUnlock() - - for _, onlineusers := range server.clients.ByNick { + for _, onlineusers := range server.clients.AllClients() { totalcount++ if onlineusers.flags[Invisible] { invisiblecount++ diff --git a/irc/snomanager.go b/irc/snomanager.go index 979e6c1d..848a87e7 100644 --- a/irc/snomanager.go +++ b/irc/snomanager.go @@ -10,7 +10,7 @@ import ( // SnoManager keeps track of which clients to send snomasks to. type SnoManager struct { - sendListMutex sync.RWMutex + sendListMutex sync.RWMutex // tier 2 sendLists map[sno.Mask]map[*Client]bool } diff --git a/irc/whowas.go b/irc/whowas.go index 8f49eaae..7a55da5c 100644 --- a/irc/whowas.go +++ b/irc/whowas.go @@ -14,7 +14,7 @@ type WhoWasList struct { start int end int - accessMutex sync.RWMutex + accessMutex sync.RWMutex // tier 2 } // WhoWas is an entry in the WhoWasList. From d5a5f939ddd61c634d49e7abdbc1e1100a078712 Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Wed, 22 Nov 2017 16:35:38 -0500 Subject: [PATCH 2/3] review fixes --- irc/client_lookup_set.go | 6 +++--- irc/nickname.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/irc/client_lookup_set.go b/irc/client_lookup_set.go index 0dabe6ca..babca39b 100644 --- a/irc/client_lookup_set.go +++ b/irc/client_lookup_set.go @@ -39,7 +39,7 @@ func ExpandUserHost(userhost string) (expanded string) { // ClientManager keeps track of clients by nick, enforcing uniqueness of casefolded nicks type ClientManager struct { sync.RWMutex // tier 2 - byNick map[string]*Client + byNick map[string]*Client } // NewClientManager returns a new ClientManager. @@ -70,7 +70,7 @@ func (clients *ClientManager) Get(nick string) *Client { } func (clients *ClientManager) removeInternal(client *Client) (removed bool) { - // requires holding ByNickMutex + // requires holding the writable Lock() oldcfnick := client.NickCasefolded() currentEntry, present := clients.byNick[oldcfnick] if present { @@ -123,7 +123,7 @@ func (clients *ClientManager) AllClients() (result []*Client) { defer clients.RUnlock() result = make([]*Client, len(clients.byNick)) i := 0 - for _, client := range(clients.byNick) { + for _, client := range clients.byNick { result[i] = client i++ } diff --git a/irc/nickname.go b/irc/nickname.go index 0599e021..8b1958e4 100644 --- a/irc/nickname.go +++ b/irc/nickname.go @@ -61,7 +61,7 @@ func performNickChange(server *Server, client *Client, target *Client, newnick s return false } - client.server.logger.Debug("nick", fmt.Sprintf("%s changed nickname to %s", origNickMask, nickname)) + client.server.logger.Debug("nick", fmt.Sprintf("%s changed nickname to %s [%s]", origNickMask, nicknameRaw, nickname)) if hadNick { target.server.snomasks.Send(sno.LocalNicks, fmt.Sprintf(ircfmt.Unescape("$%s$r changed nickname to %s"), origNick, nicknameRaw)) target.server.whoWas.Append(client) From b4907dadb9f12d2b827926fa29b99ee638dc36e6 Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Wed, 22 Nov 2017 16:55:29 -0500 Subject: [PATCH 3/3] fix a bug where the uncasefolded nickname wasn't being recorded Also, rename the nickname vars to hopefully make things clearer --- irc/nickname.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/irc/nickname.go b/irc/nickname.go index 8b1958e4..55efcb6d 100644 --- a/irc/nickname.go +++ b/irc/nickname.go @@ -32,20 +32,20 @@ func nickHandler(server *Server, client *Client, msg ircmsg.IrcMessage) bool { } func performNickChange(server *Server, client *Client, target *Client, newnick string) bool { - nicknameRaw := strings.TrimSpace(newnick) - nickname, err := CasefoldName(nicknameRaw) + nickname := strings.TrimSpace(newnick) + cfnick, err := CasefoldName(nickname) - if len(nicknameRaw) < 1 { + if len(nickname) < 1 { client.Send(nil, server.name, ERR_NONICKNAMEGIVEN, client.nick, "No nickname given") return false } - if err != nil || len(nicknameRaw) > server.Limits().NickLen || restrictedNicknames[nickname] { - client.Send(nil, server.name, ERR_ERRONEUSNICKNAME, client.nick, nicknameRaw, "Erroneous nickname") + if err != nil || len(nickname) > server.Limits().NickLen || restrictedNicknames[cfnick] { + client.Send(nil, server.name, ERR_ERRONEUSNICKNAME, client.nick, nickname, "Erroneous nickname") return false } - if target.Nick() == nicknameRaw { + if target.Nick() == nickname { return false } @@ -54,19 +54,19 @@ func performNickChange(server *Server, client *Client, target *Client, newnick s origNickMask := target.NickMaskString() err = client.server.clients.SetNick(target, nickname) if err == ErrNicknameInUse { - client.Send(nil, server.name, ERR_NICKNAMEINUSE, client.nick, nicknameRaw, "Nickname is already in use") + client.Send(nil, server.name, ERR_NICKNAMEINUSE, client.nick, nickname, "Nickname is already in use") return false } else if err != nil { client.Send(nil, server.name, ERR_UNKNOWNERROR, client.nick, "NICK", fmt.Sprintf("Could not set or change nickname: %s", err.Error())) return false } - client.server.logger.Debug("nick", fmt.Sprintf("%s changed nickname to %s [%s]", origNickMask, nicknameRaw, nickname)) + client.server.logger.Debug("nick", fmt.Sprintf("%s changed nickname to %s [%s]", origNickMask, nickname, cfnick)) if hadNick { - target.server.snomasks.Send(sno.LocalNicks, fmt.Sprintf(ircfmt.Unescape("$%s$r changed nickname to %s"), origNick, nicknameRaw)) + target.server.snomasks.Send(sno.LocalNicks, fmt.Sprintf(ircfmt.Unescape("$%s$r changed nickname to %s"), origNick, nickname)) target.server.whoWas.Append(client) for friend := range target.Friends() { - friend.Send(nil, origNickMask, "NICK", nicknameRaw) + friend.Send(nil, origNickMask, "NICK", nickname) } }