refactor monitor and /oper implementations

irc/client.go

@@ -61,8 +61,6 @@ type Client struct {
 	idleTimer          *time.Timer
 	isDestroyed        bool
 	isQuitting         bool
-	monitoring         map[string]bool
-	monitoringMutex    sync.RWMutex
 	nick               string
 	nickCasefolded     string
 	nickMaskCasefolded string
@@ -81,6 +79,7 @@ type Client struct {
 	saslValue          string
 	server             *Server
 	socket             *Socket
+	stateMutex         sync.RWMutex // generic protection for mutable state
 	timerMutex         sync.Mutex
 	username           string
 	vhost              string
@@ -101,7 +100,6 @@ func NewClient(server *Server, conn net.Conn, isTLS bool) *Client {
 		channels:       make(ChannelSet),
 		ctime:          now,
 		flags:          make(map[Mode]bool),
-		monitoring:     make(map[string]bool),
 		server:         server,
 		socket:         &socket,
 		account:        &NoAccount,
@@ -302,8 +300,8 @@ func (client *Client) Register() {
 	client.registered = true
 	client.Touch()
 
-	client.updateNickMask()
+	client.updateNickMask("")
-	client.alertMonitors()
+	client.server.monitorManager.alertMonitors(client, true)
 }
 
 // IdleTime returns how long this client's been idle.
@@ -393,19 +391,28 @@ func (client *Client) Friends(capabs ...caps.Capability) ClientSet {
 	return friends
 }
 
-// updateNick updates the casefolded nickname.
+// updateNick updates `nick` and `nickCasefolded`.
-func (client *Client) updateNick() {
+func (client *Client) updateNick(nick string) {
-	casefoldedName, err := CasefoldName(client.nick)
+	casefoldedName, err := CasefoldName(nick)
 	if err != nil {
 		log.Println(fmt.Sprintf("ERROR: Nick [%s] couldn't be casefolded... this should never happen. Printing stacktrace.", client.nick))
 		debug.PrintStack()
 	}
+	client.stateMutex.Lock()
+	client.nick = nick
 	client.nickCasefolded = casefoldedName
+	client.stateMutex.Unlock()
 }
 
 // updateNickMask updates the casefolded nickname and nickmask.
-func (client *Client) updateNickMask() {
+func (client *Client) updateNickMask(nick string) {
-	client.updateNick()
+	// on "", just regenerate the nickmask etc.
+	// otherwise, update the actual nick
+	if nick != "" {
+		client.updateNick(nick)
+	}
+
+	client.stateMutex.Lock()
 
 	if len(client.vhost) > 0 {
 		client.hostname = client.vhost
@@ -413,14 +420,17 @@ func (client *Client) updateNickMask() {
 		client.hostname = client.rawHostname
 	}
 
-	client.nickMaskString = fmt.Sprintf("%s!%s@%s", client.nick, client.username, client.hostname)
+	nickMaskString := fmt.Sprintf("%s!%s@%s", client.nick, client.username, client.hostname)
-
+	nickMaskCasefolded, err := Casefold(nickMaskString)
-	nickMaskCasefolded, err := Casefold(client.nickMaskString)
 	if err != nil {
 		log.Println(fmt.Sprintf("ERROR: Nickmask [%s] couldn't be casefolded... this should never happen. Printing stacktrace.", client.nickMaskString))
 		debug.PrintStack()
 	}
+
+	client.nickMaskString = nickMaskString
 	client.nickMaskCasefolded = nickMaskCasefolded
+
+	client.stateMutex.Unlock()
 }
 
 // AllNickmasks returns all the possible nickmasks for the client.
@@ -458,8 +468,7 @@ func (client *Client) SetNickname(nickname string) error {
 
 	err := client.server.clients.Add(client, nickname)
 	if err == nil {
-		client.nick = nickname
+		client.updateNick(nickname)
-		client.updateNick()
 	}
 	return err
 }
@@ -472,8 +481,7 @@ func (client *Client) ChangeNickname(nickname string) error {
 		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.nick = nickname
+		client.updateNickMask(nickname)
-		client.updateNickMask()
 		for friend := range client.Friends() {
 			friend.Send(nil, origNickMask, "NICK", nickname)
 		}
@@ -530,21 +538,10 @@ func (client *Client) destroy() {
 		client.server.connectionLimitsMutex.Unlock()
 	}
 
-	// remove from opers list
-	_, exists := client.server.currentOpers[client]
-	if exists {
-		delete(client.server.currentOpers, client)
-	}
-
 	// alert monitors
-	client.server.monitoringMutex.RLock()
+	client.server.monitorManager.alertMonitors(client, false)
-	for _, mClient := range client.server.monitoring[client.nickCasefolded] {
+	// clean up monitor state
-		mClient.Send(nil, client.server.name, RPL_MONOFFLINE, mClient.nick, client.nick)
+	client.server.monitorManager.clearMonitorList(client)
-	}
-	client.server.monitoringMutex.RUnlock()
-
-	// remove my monitors
-	client.clearMonitorList()
 
 	// clean up channels
 	client.server.channelJoinPartMutex.Lock()

irc/getters.go

@@ -20,3 +20,21 @@ func (server *Server) getPassword() []byte {
 	defer server.configurableStateMutex.RUnlock()
 	return server.password
 }
+
+func (client *Client) getNick() string {
+	client.stateMutex.RLock()
+	defer client.stateMutex.RUnlock()
+	return client.nick
+}
+
+func (client *Client) getNickMaskString() string {
+	client.stateMutex.RLock()
+	defer client.stateMutex.RUnlock()
+	return client.nickMaskString
+}
+
+func (client *Client) getNickCasefolded() string {
+	client.stateMutex.RLock()
+	defer client.stateMutex.RUnlock()
+	return client.nickCasefolded
+}

irc/monitor.go

@@ -4,50 +4,108 @@
 package irc
 
 import (
+	"errors"
 	"strconv"
 	"strings"
+	"sync"
 
 	"github.com/goshuirc/irc-go/ircmsg"
 )
 
-// alertMonitors alerts everyone monitoring us that we're online.
+type MonitorManager struct {
-func (client *Client) alertMonitors() {
+	sync.RWMutex
-	// get monitors
+	// client -> nicks it's watching
-	client.server.monitoringMutex.RLock()
+	watching map[*Client]map[string]bool
-	monitors := client.server.monitoring[client.nickCasefolded]
+	// nick -> clients watching it
-	client.server.monitoringMutex.RUnlock()
+	watchedby map[string]map[*Client]bool
+	// (all nicks must be normalized externally by casefolding)
+}
 
-	// alert monitors
+func NewMonitorManager() *MonitorManager {
-	for _, mClient := range monitors {
+	mm := MonitorManager{
-		// don't have to notify ourselves
+		watching:  make(map[*Client]map[string]bool),
-		if mClient != client {
+		watchedby: make(map[string]map[*Client]bool),
-			mClient.SendFromClient("", client, nil, RPL_MONONLINE, mClient.nick, client.nickMaskString)
-		}
 	}
+	return &mm
+}
+
+var MonitorLimitExceeded = errors.New("Monitor limit exceeded")
+
+// alertMonitors alerts everyone monitoring us that we're online.
+func (manager *MonitorManager) alertMonitors(client *Client, online bool) {
+	cfnick := client.getNickCasefolded()
+	nick := client.getNick()
+	var watchers []*Client
+	// safely copy the list of clients watching our nick
+	manager.RLock()
+	for client := range manager.watchedby[cfnick] {
+		watchers = append(watchers, client)
+	}
+	manager.RUnlock()
+
+	command := RPL_MONOFFLINE
+	if online {
+		command = RPL_MONONLINE
+	}
+
+	// asynchronously send all the notifications
+	go func() {
+		for _, mClient := range watchers {
+			// don't have to notify ourselves
+			if mClient != client {
+				mClient.SendFromClient("", client, nil, command, mClient.getNick(), nick)
+			}
+		}
+	}()
 }
 
 // clearMonitorList clears our MONITOR list.
-func (client *Client) clearMonitorList() {
+func (manager *MonitorManager) clearMonitorList(client *Client) {
-	// lockin' everything
+	manager.Lock()
-	client.monitoringMutex.Lock()
+	defer manager.Unlock()
-	defer client.monitoringMutex.Unlock()
-	client.server.monitoringMutex.Lock()
-	defer client.server.monitoringMutex.Unlock()
 
-	for name := range client.monitoring {
+	for nick, _ := range manager.watching[client] {
-		// just removes current client from the list
+		delete(manager.watchedby[nick], client)
-		orig := client.server.monitoring[name]
+	}
-		var index int
+	delete(manager.watching, client)
-		for i, cli := range orig {
+}
-			if cli == client {
+
-				index = i
+func (manager *MonitorManager) addMonitor(client *Client, nick string, limit int) error {
-				break
+	manager.Lock()
-			}
+	defer manager.Unlock()
-		}
+
-		client.server.monitoring[name] = append(orig[:index], orig[index+1:]...)
+	if manager.watching[client] == nil {
+		manager.watching[client] = make(map[string]bool)
+	}
+	if manager.watchedby[nick] == nil {
+		manager.watchedby[nick] = make(map[*Client]bool)
 	}
 
-	client.monitoring = make(map[string]bool)
+	if len(manager.watching[client]) >= limit {
+		return MonitorLimitExceeded
+	}
+
+	manager.watching[client][nick] = true
+	manager.watchedby[nick][client] = true
+	return nil
+}
+
+func (manager *MonitorManager) removeMonitor(client *Client, nick string) error {
+	manager.Lock()
+	defer manager.Unlock()
+	// deleting from nil maps is fine
+	delete(manager.watching[client], nick)
+	delete(manager.watchedby[nick], client)
+	return nil
+}
+
+func (manager *MonitorManager) listMonitors(client *Client) (nicks []string) {
+	manager.RLock()
+	defer manager.RUnlock()
+	for nick := range manager.watching[client] {
+		nicks = append(nicks, nick)
+	}
+	return nicks
 }
 
 var (
@@ -64,7 +122,7 @@ func monitorHandler(server *Server, client *Client, msg ircmsg.IrcMessage) bool
 	handler, exists := metadataSubcommands[strings.ToLower(msg.Params[0])]
 
 	if !exists {
-		client.Send(nil, server.name, ERR_UNKNOWNERROR, client.nick, "MONITOR", msg.Params[0], "Unknown subcommand")
+		client.Send(nil, server.name, ERR_UNKNOWNERROR, client.getNick(), "MONITOR", msg.Params[0], "Unknown subcommand")
 		return false
 	}
 
@@ -73,49 +131,17 @@ func monitorHandler(server *Server, client *Client, msg ircmsg.IrcMessage) bool
 
 func monitorRemoveHandler(server *Server, client *Client, msg ircmsg.IrcMessage) bool {
 	if len(msg.Params) < 2 {
-		client.Send(nil, server.name, ERR_NEEDMOREPARAMS, client.nick, msg.Command, "Not enough parameters")
+		client.Send(nil, server.name, ERR_NEEDMOREPARAMS, client.getNick(), msg.Command, "Not enough parameters")
 		return false
 	}
 
 	targets := strings.Split(msg.Params[1], ",")
-	for len(targets) > 0 {
+	for _, target := range targets {
-		// check name length
+		cfnick, err := CasefoldName(target)
-		if len(targets[0]) < 1 {
-			targets = targets[1:]
-			continue
-		}
-
-		// remove target
-		casefoldedTarget, err := CasefoldName(targets[0])
 		if err != nil {
-			// skip silently I guess
-			targets = targets[1:]
 			continue
 		}
-
+		server.monitorManager.removeMonitor(client, cfnick)
-		client.monitoringMutex.Lock()
-		client.server.monitoringMutex.Lock()
-
-		if client.monitoring[casefoldedTarget] {
-			// just removes current client from the list
-			orig := server.monitoring[casefoldedTarget]
-			var index int
-			for i, cli := range orig {
-				if cli == client {
-					index = i
-					break
-				}
-			}
-			server.monitoring[casefoldedTarget] = append(orig[:index], orig[index+1:]...)
-
-			delete(client.monitoring, casefoldedTarget)
-		}
-
-		client.monitoringMutex.Unlock()
-		client.server.monitoringMutex.Unlock()
-
-		// remove first element of targets list
-		targets = targets[1:]
 	}
 
 	return false
@@ -123,88 +149,68 @@ func monitorRemoveHandler(server *Server, client *Client, msg ircmsg.IrcMessage)
 
 func monitorAddHandler(server *Server, client *Client, msg ircmsg.IrcMessage) bool {
 	if len(msg.Params) < 2 {
-		client.Send(nil, server.name, ERR_NEEDMOREPARAMS, client.nick, msg.Command, "Not enough parameters")
+		client.Send(nil, server.name, ERR_NEEDMOREPARAMS, client.getNick(), msg.Command, "Not enough parameters")
 		return false
 	}
 
 	var online []string
 	var offline []string
 
-	targets := strings.Split(msg.Params[1], ",")
+	limit := server.getLimits().MonitorEntries
-	for len(targets) > 0 {
-		// check name length
-		if len(targets[0]) < 1 || len(targets[0]) > server.limits.NickLen {
-			targets = targets[1:]
-			continue
-		}
 
-		// check the monitor list length
+	targets := strings.Split(msg.Params[1], ",")
-		if len(client.monitoring) >= server.limits.MonitorEntries {
+	for _, target := range targets {
-			client.Send(nil, server.name, ERR_MONLISTFULL, client.nick, strconv.Itoa(server.limits.MonitorEntries), strings.Join(targets, ","))
+		// check name length
-			break
+		if len(target) < 1 || len(targets) > server.limits.NickLen {
+			continue
 		}
 
 		// add target
 		casefoldedTarget, err := CasefoldName(targets[0])
 		if err != nil {
-			// skip silently I guess
-			targets = targets[1:]
 			continue
 		}
 
-		client.monitoringMutex.Lock()
+		err = server.monitorManager.addMonitor(client, casefoldedTarget, limit)
-		client.server.monitoringMutex.Lock()
+		if err == MonitorLimitExceeded {
-
+			client.Send(nil, server.name, ERR_MONLISTFULL, client.getNick(), strconv.Itoa(server.limits.MonitorEntries), strings.Join(targets, ","))
-		if !client.monitoring[casefoldedTarget] {
+			break
-			client.monitoring[casefoldedTarget] = true
+		} else if err != nil {
-
+			continue
-			orig := server.monitoring[casefoldedTarget]
-			server.monitoring[casefoldedTarget] = append(orig, client)
 		}
 
-		client.monitoringMutex.Unlock()
-		client.server.monitoringMutex.Unlock()
-
 		// add to online / offline lists
-		target := server.clients.Get(casefoldedTarget)
+		if target := server.clients.Get(casefoldedTarget); target == nil {
-		if target == nil {
 			offline = append(offline, targets[0])
 		} else {
-			online = append(online, target.nickMaskString)
+			online = append(online, target.getNick())
 		}
-
-		// remove first element of targets list
-		targets = targets[1:]
 	}
 
 	if len(online) > 0 {
-		client.Send(nil, server.name, RPL_MONONLINE, client.nick, strings.Join(online, ","))
+		client.Send(nil, server.name, RPL_MONONLINE, client.getNick(), strings.Join(online, ","))
 	}
 	if len(offline) > 0 {
-		client.Send(nil, server.name, RPL_MONOFFLINE, client.nick, strings.Join(offline, ","))
+		client.Send(nil, server.name, RPL_MONOFFLINE, client.getNick(), strings.Join(offline, ","))
 	}
 
 	return false
 }
 
 func monitorClearHandler(server *Server, client *Client, msg ircmsg.IrcMessage) bool {
-	client.clearMonitorList()
+	server.monitorManager.clearMonitorList(client)
-
 	return false
 }
 
 func monitorListHandler(server *Server, client *Client, msg ircmsg.IrcMessage) bool {
-	var monitorList []string
+	monitorList := server.monitorManager.listMonitors(client)
-	client.monitoringMutex.RLock()
-	for name := range client.monitoring {
-		monitorList = append(monitorList, name)
-	}
-	client.monitoringMutex.RUnlock()
 
 	for _, line := range argsToStrings(maxLastArgLength, monitorList, ",") {
-		client.Send(nil, server.name, RPL_MONLIST, client.nick, line)
+		client.Send(nil, server.name, RPL_MONLIST, client.getNick(), line)
 	}
 
+	client.Send(nil, server.name, RPL_ENDOFMONLIST, "End of MONITOR list")
+
 	return false
 }
 
@@ -212,27 +218,25 @@ func monitorStatusHandler(server *Server, client *Client, msg ircmsg.IrcMessage)
 	var online []string
 	var offline []string
 
-	client.monitoringMutex.RLock()
+	monitorList := server.monitorManager.listMonitors(client)
-	monitoring := client.monitoring
-	client.monitoringMutex.RUnlock()
 
-	for name := range monitoring {
+	for _, name := range monitorList {
 		target := server.clients.Get(name)
 		if target == nil {
 			offline = append(offline, name)
 		} else {
-			online = append(online, target.nickMaskString)
+			online = append(online, target.getNick())
 		}
 	}
 
 	if len(online) > 0 {
 		for _, line := range argsToStrings(maxLastArgLength, online, ",") {
-			client.Send(nil, server.name, RPL_MONONLINE, client.nick, line)
+			client.Send(nil, server.name, RPL_MONONLINE, client.getNick(), line)
 		}
 	}
 	if len(offline) > 0 {
 		for _, line := range argsToStrings(maxLastArgLength, offline, ",") {
-			client.Send(nil, server.name, RPL_MONOFFLINE, client.nick, line)
+			client.Send(nil, server.name, RPL_MONOFFLINE, client.getNick(), line)
 		}
 	}
 

irc/nickname.go

@@ -57,7 +57,7 @@ func nickHandler(server *Server, client *Client, msg ircmsg.IrcMessage) bool {
 		return false
 	}
 	if client.registered {
-		client.alertMonitors()
+		client.server.monitorManager.alertMonitors(client, true)
 	}
 	server.tryRegister(client)
 	return false

irc/server.go

@@ -94,7 +94,6 @@ type Server struct {
 	connectionThrottle           *ConnectionThrottle
 	connectionThrottleMutex      sync.Mutex // used when affecting the connection limiter, to make sure rehashing doesn't make things go out-of-whack
 	ctime                        time.Time
-	currentOpers                 map[*Client]bool
 	defaultChannelModes          Modes
 	dlines                       *DLineManager
 	isupport                     *ISupportList
@@ -103,8 +102,7 @@ type Server struct {
 	listeners                    map[string]*ListenerWrapper
 	logger                       *logger.Manager
 	MaxSendQBytes                uint64
-	monitoring                   map[string][]*Client
+	monitorManager               *MonitorManager
-	monitoringMutex              sync.RWMutex
 	motdLines                    []string
 	name                         string
 	nameCasefolded               string
@@ -155,10 +153,9 @@ func NewServer(config *Config, logger *logger.Manager) (*Server, error) {
 		channels:           *NewChannelNameMap(),
 		clients:            NewClientLookupSet(),
 		commands:           make(chan Command),
-		currentOpers:       make(map[*Client]bool),
 		listeners:          make(map[string]*ListenerWrapper),
 		logger:             logger,
-		monitoring:         make(map[string][]*Client),
+		monitorManager:     NewMonitorManager(),
 		newConns:           make(chan clientConn),
 		registeredChannels: make(map[string]*RegisteredChannel),
 		rehashSignal:       make(chan os.Signal, 1),
@@ -1143,36 +1140,36 @@ func operHandler(server *Server, client *Client, msg ircmsg.IrcMessage) bool {
 		client.Send(nil, server.name, ERR_UNKNOWNERROR, "OPER", "You're already opered-up!")
 		return false
 	}
-	hash := server.operators[name].Pass
+	server.configurableStateMutex.RLock()
+	oper := server.operators[name]
+	server.configurableStateMutex.RUnlock()
+
 	password := []byte(msg.Params[1])
-
+	err = ComparePassword(oper.Pass, password)
-	err = ComparePassword(hash, password)
+	if (oper.Pass == nil) || (err != nil) {
-
-	if (hash == nil) || (err != nil) {
 		client.Send(nil, server.name, ERR_PASSWDMISMATCH, client.nick, "Password incorrect")
 		return true
 	}
 
 	client.flags[Operator] = true
 	client.operName = name
-	client.class = server.operators[name].Class
+	client.class = oper.Class
-	server.currentOpers[client] = true
+	client.whoisLine = oper.WhoisLine
-	client.whoisLine = server.operators[name].WhoisLine
 
 	// push new vhost if one is set
-	if len(server.operators[name].Vhost) > 0 {
+	if len(oper.Vhost) > 0 {
 		for fClient := range client.Friends(caps.ChgHost) {
-			fClient.SendFromClient("", client, nil, "CHGHOST", client.username, server.operators[name].Vhost)
+			fClient.SendFromClient("", client, nil, "CHGHOST", client.username, oper.Vhost)
 		}
 		// CHGHOST requires prefix nickmask to have original hostname, so do that before updating nickmask
-		client.vhost = server.operators[name].Vhost
+		client.vhost = oper.Vhost
-		client.updateNickMask()
+		client.updateNickMask("")
 	}
 
 	// set new modes
 	var applied ModeChanges
-	if 0 < len(server.operators[name].Modes) {
+	if 0 < len(oper.Modes) {
-		modeChanges, unknownChanges := ParseUserModeChanges(strings.Split(server.operators[name].Modes, " ")...)
+		modeChanges, unknownChanges := ParseUserModeChanges(strings.Split(oper.Modes, " ")...)
 		applied = client.applyUserModeChanges(true, modeChanges)
 		if 0 < len(unknownChanges) {
 			var runes string
@@ -1257,12 +1254,8 @@ func (server *Server) applyConfig(config *Config, initial bool) error {
 	if err != nil {
 		return fmt.Errorf("Error rehashing config file opers: %s", err.Error())
 	}
-	for client := range server.currentOpers {
+
-		_, exists := opers[client.operName]
+	// TODO: support rehash of existing operator perms?
-		if !exists {
-			return fmt.Errorf("Oper [%s] no longer exists (used by client [%s])", client.operName, client.nickMaskString)
-		}
-	}
 
 	// sanity checks complete, start modifying server state