Nick locking (entirely broken, needs to be completely redesigned)

irc/client.go

@@ -6,6 +6,7 @@
 package irc
 
 import (
+	"errors"
 	"fmt"
 	"log"
 	"net"
@@ -13,6 +14,8 @@ import (
 	"strconv"
 	"time"
 
+	"strings"
+
 	"github.com/DanielOaks/girc-go/ircmsg"
 	"github.com/DanielOaks/go-ident"
 )
@@ -25,6 +28,7 @@ const (
 
 var (
 	TIMEOUT_STATED_SECONDS = strconv.Itoa(int((IDLE_TIMEOUT + QUIT_TIMEOUT).Seconds()))
+	ErrNickAlreadySet      = errors.New("Nickname is already set")
 )
 
 // Client is an IRC client.
@@ -142,12 +146,14 @@ func (client *Client) run() {
 	client.rawHostname = AddrLookupHostname(client.socket.conn.RemoteAddr())
 
 	//TODO(dan): Make this a socketreactor from ircbnc
+	fmt.Println("START", &client)
 	for {
 		line, err = client.socket.Read()
 		if err != nil {
 			client.Quit("connection closed")
 			break
 		}
+		fmt.Println("  LINE", &client, strings.TrimSpace(line))
 
 		msg, err = ircmsg.ParseLine(line)
 		if err != nil {
@@ -157,6 +163,7 @@ func (client *Client) run() {
 
 		cmd, exists := Commands[msg.Command]
 		if !exists {
+			fmt.Println("    BADLINE", &client, strings.TrimSpace(line))
 			if len(msg.Command) > 0 {
 				client.Send(nil, client.server.name, ERR_UNKNOWNCOMMAND, client.nick, msg.Command, "Unknown command")
 			} else {
@@ -164,11 +171,15 @@ func (client *Client) run() {
 			}
 			continue
 		}
+		fmt.Println("    GUDLINE", &client, strings.TrimSpace(line))
 
 		isExiting = cmd.Run(client.server, client, msg)
+		fmt.Println("      CMDRUN", &client, strings.TrimSpace(line))
 		if isExiting || client.isQuitting {
+			fmt.Println("        BREAKING", &client, strings.TrimSpace(line))
 			break
 		}
+		fmt.Println("        CONTINUE", &client, strings.TrimSpace(line))
 	}
 
 	// ensure client connection gets closed
@@ -349,18 +360,20 @@ func (client *Client) updateNickMask() {
 }
 
 // SetNickname sets the very first nickname for the client.
-func (client *Client) SetNickname(nickname string) {
+func (client *Client) SetNickname(nickname string) error {
 	if client.HasNick() {
 		Log.error.Printf("%s nickname already set!", client.nickMaskString)
-		return
+		return ErrNickAlreadySet
 	}
+
 	client.nick = nickname
 	client.updateNick()
 	client.server.clients.Add(client)
+	return nil
 }
 
 // ChangeNickname changes the existing nickname of the client.
-func (client *Client) ChangeNickname(nickname string) {
+func (client *Client) ChangeNickname(nickname string) error {
 	origNickMask := client.nickMaskString
 	client.server.clients.Remove(client)
 	client.server.whoWas.Append(client)
@@ -370,6 +383,7 @@ func (client *Client) ChangeNickname(nickname string) {
 	for friend := range client.Friends() {
 		friend.Send(nil, origNickMask, "NICK", nickname)
 	}
+	return nil
 }
 
 func (client *Client) Quit(message string) {

irc/client_lookup_set.go

@@ -11,6 +11,10 @@ import (
 	"regexp"
 	"strings"
 
+	"sync"
+
+	"runtime/debug"
+
 	"github.com/DanielOaks/girc-go/ircmatch"
 )
 
@@ -33,7 +37,24 @@ func ExpandUserHost(userhost string) (expanded string) {
 	return
 }
 
+type LoggingMutex struct {
+	mutex sync.Mutex
+}
+
+func (lm *LoggingMutex) Lock() {
+	lm.mutex.Lock()
+	fmt.Println("--- locked, stack:")
+	debug.PrintStack()
+}
+
+func (lm *LoggingMutex) Unlock() {
+	fmt.Println("--- unlocking")
+	lm.mutex.Unlock()
+	fmt.Println("--- unlocked")
+}
+
 type ClientLookupSet struct {
+	ByNickMutex LoggingMutex
 	ByNick      map[string]*Client
 }
 
@@ -44,34 +65,62 @@ func NewClientLookupSet() *ClientLookupSet {
 }
 
 func (clients *ClientLookupSet) Count() int {
-	return len(clients.ByNick)
+	clients.ByNickMutex.Lock()
+	count := len(clients.ByNick)
+	clients.ByNickMutex.Unlock()
+	return count
 }
 
+//TODO(dan): wouldn't it be best to always use Get rather than this?
 func (clients *ClientLookupSet) Has(nick string) bool {
 	casefoldedName, err := CasefoldName(nick)
 	if err == nil {
 		return false
 	}
+	clients.ByNickMutex.Lock()
 	_, exists := clients.ByNick[casefoldedName]
+	clients.ByNickMutex.Unlock()
 	return exists
 }
 
+func (clients *ClientLookupSet) getNoMutex(nick string) *Client {
+	casefoldedName, err := CasefoldName(nick)
+	if err == nil {
+		cli := clients.ByNick[casefoldedName]
+		return cli
+	}
+	return nil
+}
+
 func (clients *ClientLookupSet) Get(nick string) *Client {
 	casefoldedName, err := CasefoldName(nick)
 	if err == nil {
-		return clients.ByNick[casefoldedName]
+		clients.ByNickMutex.Lock()
+		cli := clients.ByNick[casefoldedName]
+		clients.ByNickMutex.Unlock()
+		return cli
 	}
 	return nil
 }
 
 func (clients *ClientLookupSet) Add(client *Client) error {
+	fmt.Println("Initial nick:", client.nick)
 	if !client.HasNick() {
 		return ErrNickMissing
 	}
-	if clients.Get(client.nick) != nil {
+	fmt.Println("- getting lock:", client.nick)
+	clients.ByNickMutex.Lock()
+	fmt.Println("- got lock:", client.nick)
+	if clients.getNoMutex(client.nick) != nil {
+		fmt.Println("- already exists:", client.nick)
+		clients.ByNickMutex.Unlock()
 		return ErrNicknameInUse
 	}
+	fmt.Println("- adding:", client.nick)
 	clients.ByNick[client.nickCasefolded] = client
+	fmt.Println("- set:", client.nick)
+	clients.ByNickMutex.Unlock()
+	fmt.Println("- released lock:", client.nick)
 	return nil
 }
 
@@ -79,16 +128,19 @@ func (clients *ClientLookupSet) Remove(client *Client) error {
 	if !client.HasNick() {
 		return ErrNickMissing
 	}
-	if clients.Get(client.nick) != client {
+	if clients.getNoMutex(client.nick) != client {
 		return ErrNicknameMismatch
 	}
+	clients.ByNickMutex.Lock()
 	delete(clients.ByNick, client.nickCasefolded)
+	clients.ByNickMutex.Unlock()
 	return nil
 }
 
 func (clients *ClientLookupSet) AllWithCaps(caps ...Capability) (set ClientSet) {
 	set = make(ClientSet)
 
+	clients.ByNickMutex.Lock()
 	var client *Client
 	for _, client = range clients.ByNick {
 		// make sure they have all the required caps
@@ -100,6 +152,7 @@ func (clients *ClientLookupSet) AllWithCaps(caps ...Capability) (set ClientSet)
 
 		set.Add(client)
 	}
+	clients.ByNickMutex.Unlock()
 
 	return set
 }
@@ -113,11 +166,13 @@ func (clients *ClientLookupSet) FindAll(userhost string) (set ClientSet) {
 	}
 	matcher := ircmatch.MakeMatch(userhost)
 
+	clients.ByNickMutex.Lock()
 	for _, client := range clients.ByNick {
 		if matcher.Match(client.nickMaskCasefolded) {
 			set.Add(client)
 		}
 	}
+	clients.ByNickMutex.Unlock()
 
 	return set
 }
@@ -128,14 +183,18 @@ func (clients *ClientLookupSet) Find(userhost string) *Client {
 		return nil
 	}
 	matcher := ircmatch.MakeMatch(userhost)
+	var matchedClient *Client
 
+	clients.ByNickMutex.Lock()
 	for _, client := range clients.ByNick {
 		if matcher.Match(client.nickMaskCasefolded) {
-			return client
+			matchedClient = client
+			break
 		}
 	}
+	clients.ByNickMutex.Unlock()
 
-	return nil
+	return matchedClient
 }
 
 //

irc/nickname.go

@@ -34,18 +34,23 @@ func nickHandler(server *Server, client *Client, msg ircmsg.IrcMessage) bool {
 		return false
 	}
 
-	//TODO(dan): There's probably some races here, we should be changing this in the primary server thread
+	// bleh, this will be replaced and done below
 	target := server.clients.Get(nickname)
 	if target != nil && target != client {
 		client.Send(nil, server.name, ERR_NICKNAMEINUSE, client.nick, nicknameRaw, "Nickname is already in use")
 		return false
 	}
-
 	if client.registered {
-		client.ChangeNickname(nicknameRaw)
+		err = client.ChangeNickname(nicknameRaw)
-		client.alertMonitors()
 	} else {
-		client.SetNickname(nicknameRaw)
+		err = client.SetNickname(nicknameRaw)
+	}
+	if err != nil {
+		client.Send(nil, server.name, ERR_NICKNAMEINUSE, client.nick, nicknameRaw, "Nickname is already in use")
+		return false
+	}
+	if client.registered {
+		client.alertMonitors()
 	}
 	server.tryRegister(client)
 	return false

irc/server.go

@@ -343,9 +343,11 @@ func loadChannelList(channel *Channel, list string, maskMode ChannelMode) {
 
 func (server *Server) Shutdown() {
 	//TODO(dan): Make sure we disallow new nicks
+	server.clients.ByNickMutex.Lock()
 	for _, client := range server.clients.ByNick {
 		client.Notice("Server is shutting down")
 	}
+	server.clients.ByNickMutex.Unlock()
 
 	if err := server.store.Close(); err != nil {
 		Log.error.Println("Server.Shutdown store.Close: error:", err)
@@ -565,6 +567,7 @@ func (server *Server) wslisten(addr string, tlsMap map[string]*TLSListenConfig)
 func (server *Server) tryRegister(c *Client) {
 	if c.registered || !c.HasNick() || !c.HasUsername() ||
 		(c.capState == CapNegotiating) {
+		fmt.Println("Try Reg:", &c, c.registered, c.HasNick(), c.HasUsername(), c.capState == CapNegotiating, c.capState)
 		return
 	}
 	c.Register()
@@ -1075,12 +1078,14 @@ func (server *Server) rehash() error {
 	server.connectionLimitsMutex.Lock()
 	server.connectionLimits = connectionLimits
 
+	server.clients.ByNickMutex.Lock()
 	for _, client := range server.clients.ByNick {
 		ipaddr := net.ParseIP(IPString(client.socket.conn.RemoteAddr()))
 		if ipaddr != nil {
 			server.connectionLimits.AddClient(ipaddr, true)
 		}
 	}
+	server.clients.ByNickMutex.Unlock()
 	server.connectionLimitsMutex.Unlock()
 
 	// setup new and removed caps
@@ -1147,12 +1152,14 @@ func (server *Server) rehash() error {
 	newISupportReplies := oldISupportList.GetDifference(server.isupport)
 
 	// push new info to all of our clients
+	server.clients.ByNickMutex.Lock()
 	for _, sClient := range server.clients.ByNick {
 		for _, tokenline := range newISupportReplies {
 			// ugly trickery ahead
 			sClient.Send(nil, server.name, RPL_ISUPPORT, append([]string{sClient.nick}, tokenline...)...)
 		}
 	}
+	server.clients.ByNickMutex.Unlock()
 
 	// destroy old listeners
 	tlsListeners := config.TLSListeners()