Merge pull request #168 from slingamn/clientmanager

refactor ClientManager

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 <procid>
 
 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.

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
 ```

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

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

irc/client_lookup_set.go

@@ -18,9 +18,8 @@ import (
 )
 
 var (
-	ErrNickMissing      = errors.New("nick missing")
+	ErrNickMissing   = errors.New("nick missing")
-	ErrNicknameInUse    = errors.New("nickname in use")
+	ErrNicknameInUse = errors.New("nickname in use")
-	ErrNicknameMismatch = errors.New("nickname mismatch")
 )
 
 // 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.
+// ClientManager keeps track of clients by nick, enforcing uniqueness of casefolded nicks
-type ClientLookupSet struct {
+type ClientManager struct {
-	ByNickMutex sync.RWMutex
+	sync.RWMutex // tier 2
-	ByNick      map[string]*Client
+	byNick       map[string]*Client
 }
 
-// NewClientLookupSet returns a new lookup set.
+// NewClientManager returns a new ClientManager.
-func NewClientLookupSet() *ClientLookupSet {
+func NewClientManager() *ClientManager {
-	return &ClientLookupSet{
+	return &ClientManager{
-		ByNick: make(map[string]*Client),
+		byNick: make(map[string]*Client),
 	}
 }
 
-// Count returns how many clients are in the lookup set.
+// Count returns how many clients are in the manager.
-func (clients *ClientLookupSet) Count() int {
+func (clients *ClientManager) Count() int {
-	clients.ByNickMutex.RLock()
+	clients.RLock()
-	defer clients.ByNickMutex.RUnlock()
+	defer clients.RUnlock()
-	count := len(clients.ByNick)
+	count := len(clients.byNick)
 	return count
 }
 
-// Has returns whether or not the given client exists.
+// Get retrieves a client from the manager, if they exist.
-//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 *ClientManager) Get(nick string) *Client {
-func (clients *ClientLookupSet) Has(nick string) bool {
 	casefoldedName, err := CasefoldName(nick)
 	if err == nil {
-		return false
+		clients.RLock()
-	}
+		defer clients.RUnlock()
-	clients.ByNickMutex.RLock()
+		cli := clients.byNick[casefoldedName]
-	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]
 		return cli
 	}
 	return nil
 }
 
-// Get retrieves a client from the set, if they exist.
+func (clients *ClientManager) removeInternal(client *Client) (removed bool) {
-func (clients *ClientLookupSet) Get(nick string) *Client {
+	// requires holding the writable Lock()
-	casefoldedName, err := CasefoldName(nick)
+	oldcfnick := client.NickCasefolded()
-	if err == nil {
+	currentEntry, present := clients.byNick[oldcfnick]
-		clients.ByNickMutex.RLock()
+	if present {
-		defer clients.ByNickMutex.RUnlock()
+		if currentEntry == client {
-		cli := clients.ByNick[casefoldedName]
+			delete(clients.byNick, oldcfnick)
-		return cli
+			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
+	return
-}
-
-// 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
 }
 
 // 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()
+	clients.removeInternal(client)
-	defer clients.ByNickMutex.Unlock()
-	if clients.getNoMutex(client.nick) != client {
-		return ErrNicknameMismatch
-	}
-	delete(clients.ByNick, client.nickCasefolded)
 	return nil
 }
 
-// Replace renames an existing client in the lookup set.
+// SetNick sets a client's nickname, validating it against nicknames in use
-func (clients *ClientLookupSet) Replace(oldNick, newNick string, client *Client) error {
+func (clients *ClientManager) SetNick(client *Client, newNick string) error {
-	// get casefolded nicknames
+	newcfnick, err := CasefoldName(newNick)
-	oldNick, err := CasefoldName(oldNick)
-	if err != nil {
-		return err
-	}
-	newNick, err = CasefoldName(newNick)
 	if err != nil {
 		return err
 	}
 
-	// remove and replace
+	clients.Lock()
-	clients.ByNickMutex.Lock()
+	defer clients.Unlock()
-	defer clients.ByNickMutex.Unlock()
 
-	oldClient := clients.ByNick[newNick]
+	clients.removeInternal(client)
-	if oldClient == nil || oldClient == client {
+	currentNewEntry := clients.byNick[newcfnick]
-		// whoo
+	// the client may just be changing case
-	} else {
+	if currentNewEntry != nil && currentNewEntry != client {
 		return ErrNicknameInUse
 	}
-
+	clients.byNick[newcfnick] = client
-	if oldNick == newNick {
+	client.updateNickMask(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
 	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()
+	clients.RLock()
-	defer clients.ByNickMutex.RUnlock()
+	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()
+	clients.RLock()
-	defer clients.ByNickMutex.RUnlock()
+	defer clients.RUnlock()
-	for _, client := range clients.ByNick {
+	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()
+	clients.RLock()
-	defer clients.ByNickMutex.RUnlock()
+	defer clients.RUnlock()
-	for _, client := range clients.ByNick {
+	for _, client := range clients.byNick {
 		if matcher.Match(client.nickMaskCasefolded) {
 			matchedClient = client
 			break

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.AllClients() {
-		for _, mcl := range server.clients.ByNick {
 			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

irc/idletimer.go

@@ -29,7 +29,7 @@ const (
 )
 
 type IdleTimer struct {
-	sync.Mutex
+	sync.Mutex // tier 1
 
 	// immutable after construction
 	registerTimeout time.Duration

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.AllClients() {
-		for _, mcl := range server.clients.ByNick {
 			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

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

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,79 +28,63 @@ 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])
-	nickname, err := CasefoldName(nicknameRaw)
-
-	if len(nicknameRaw) < 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")
-		return false
-	}
-
-	if client.nick == nickname {
-		return false
-	}
-
-	// bleh, this will be replaced and done below
-	if client.registered {
-		err = client.ChangeNickname(nicknameRaw)
-	} else {
-		err = client.SetNickname(nicknameRaw)
-	}
-	if err == ErrNicknameInUse {
-		client.Send(nil, server.name, ERR_NICKNAMEINUSE, client.nick, nicknameRaw, "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
-	}
-	if client.registered {
-		client.server.monitorManager.AlertAbout(client, true)
-	}
-	server.tryRegister(client)
-	return false
 }
 
-// SANICK <oldnick> <nickname>
+func performNickChange(server *Server, client *Client, target *Client, newnick string) bool {
-func sanickHandler(server *Server, client *Client, msg ircmsg.IrcMessage) bool {
+	nickname := strings.TrimSpace(newnick)
-	if !client.authorized {
+	cfnick, err := CasefoldName(nickname)
-		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] {
+	if err != nil || len(nickname) > server.Limits().NickLen || restrictedNicknames[cfnick] {
-		client.Send(nil, server.name, ERR_ERRONEUSNICKNAME, client.nick, msg.Params[0], "Erroneous nickname")
+		client.Send(nil, server.name, ERR_ERRONEUSNICKNAME, client.nick, nickname, "Erroneous nickname")
 		return false
 	}
 
-	if client.nick == msg.Params[1] {
+	if target.Nick() == nickname {
 		return false
 	}
 
-	target := server.clients.Get(oldnick)
+	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, 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, nickname, cfnick))
+	if hadNick {
+		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", nickname)
+		}
+	}
+
+	if target.registered {
+		client.server.monitorManager.AlertAbout(target, true)
+	} else {
+		server.tryRegister(target)
+	}
+	return false
+}
+
+// SANICK <oldnick> <nickname>
+func sanickHandler(server *Server, client *Client, msg ircmsg.IrcMessage) bool {
+	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
 	}
-
+	return performNickChange(server, client, target, msg.Params[1])
-	//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
 }

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.AllClients() {
-	for _, client := range server.clients.ByNick {
 		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.AllClients() {
-		for _, sClient := range server.clients.ByNick {
 			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.AllClients() {
-		for _, sClient := range server.clients.ByNick {
 			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()
+	for _, onlineusers := range server.clients.AllClients() {
-	defer server.clients.ByNickMutex.RUnlock()
-
-	for _, onlineusers := range server.clients.ByNick {
 		totalcount++
 		if onlineusers.flags[Invisible] {
 			invisiblecount++

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
 }
 

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.