fix #1824

Make (*Client).Sessions() lock-free

irc/client.go

@@ -15,6 +15,7 @@ import (
 	"sync"
 	"sync/atomic"
 	"time"
+	"unsafe"
 
 	ident "github.com/ergochat/go-ident"
 	"github.com/ergochat/irc-go/ircfmt"
@@ -104,7 +105,7 @@ type Client struct {
 	registrationTimer  *time.Timer
 	server             *Server
 	skeleton           string
-	sessions           []*Session
+	sessions           unsafe.Pointer
 	stateMutex         sync.RWMutex // tier 1
 	alwaysOn           bool
 	username           string
@@ -360,7 +361,7 @@ func (server *Server) RunClient(conn IRCConn) {
 		isTor:      wConn.Config.Tor,
 		hideSTS:    wConn.Config.Tor || wConn.Config.HideSTS,
 	}
-	client.sessions = []*Session{session}
+	client.setSessions([]*Session{session})
 
 	session.resetFakelag()
 
@@ -1018,9 +1019,7 @@ func (client *Client) FriendsMonitors(capabs ...caps.Capability) (result map[*Se
 
 // helper for Friends
 func addFriendsToSet(set map[*Session]empty, client *Client, capabs ...caps.Capability) {
-	client.stateMutex.RLock()
-	defer client.stateMutex.RUnlock()
-	for _, session := range client.sessions {
+	for _, session := range client.Sessions() {
 		if session.capabilities.HasAll(capabs...) {
 			set[session] = empty{}
 		}
@@ -1174,7 +1173,7 @@ func (client *Client) Quit(message string, session *Session) {
 	if session != nil {
 		sessions = []*Session{session}
 	} else {
-		sessions = client.sessions
+		sessions = client.Sessions()
 	}
 
 	for _, session := range sessions {
@@ -1213,8 +1212,8 @@ func (client *Client) destroy(session *Session) {
 
 	var remainingSessions int
 	if session == nil {
-		sessionsToDestroy = client.sessions
-		client.sessions = nil
+		sessionsToDestroy = client.Sessions()
+		client.setSessions(nil)
 		remainingSessions = 0
 	} else {
 		sessionRemoved, remainingSessions = client.removeSession(session)
@@ -1241,7 +1240,7 @@ func (client *Client) destroy(session *Session) {
 	shouldDestroy := !client.destroyed && remainingSessions == 0 && !alwaysOn
 	// decrement stats on a true destroy, or for the removal of the last connected session
 	// of an always-on client
-	shouldDecrement := shouldDestroy || (alwaysOn && len(sessionsToDestroy) != 0 && len(client.sessions) == 0)
+	shouldDecrement := shouldDestroy || (alwaysOn && len(sessionsToDestroy) != 0 && len(client.Sessions()) == 0)
 	if shouldDestroy {
 		// if it's our job to destroy it, don't let anyone else try
 		client.destroyed = true

irc/getters.go

@@ -48,10 +48,11 @@ func (server *Server) SetDefcon(defcon uint32) {
 }
 
 func (client *Client) Sessions() (sessions []*Session) {
-	client.stateMutex.RLock()
-	sessions = client.sessions
-	client.stateMutex.RUnlock()
-	return
+	ptr := atomic.LoadPointer(&client.sessions)
+	if ptr == nil {
+		return nil
+	}
+	return *((*[]*Session)(ptr))
 }
 
 type SessionData struct {
@@ -67,12 +68,14 @@ type SessionData struct {
 }
 
 func (client *Client) AllSessionData(currentSession *Session, hasPrivs bool) (data []SessionData, currentIndex int) {
-	currentIndex = -1
 	client.stateMutex.RLock()
 	defer client.stateMutex.RUnlock()
 
-	data = make([]SessionData, len(client.sessions))
-	for i, session := range client.sessions {
+	currentIndex = -1
+	sessions := client.Sessions()
+
+	data = make([]SessionData, len(sessions))
+	for i, session := range sessions {
 		if session == currentSession {
 			currentIndex = i
 		}
@@ -107,40 +110,49 @@ func (client *Client) AddSession(session *Session) (success bool, numSessions in
 		return
 	}
 	// success, attach the new session to the client
+	sessions := client.Sessions()
 	session.client = client
 	session.sessionID = client.nextSessionID
 	client.nextSessionID++
-	newSessions := make([]*Session, len(client.sessions)+1)
-	copy(newSessions, client.sessions)
+	newSessions := make([]*Session, len(sessions)+1)
+	copy(newSessions, sessions)
 	newSessions[len(newSessions)-1] = session
 	if client.accountSettings.AutoreplayMissed || session.deviceID != "" {
 		lastSeen = client.lastSeen[session.deviceID]
 		client.setLastSeen(time.Now().UTC(), session.deviceID)
 	}
-	client.sessions = newSessions
+	client.setSessions(newSessions)
 	// TODO(#1551) there should be a cap to opt out of this behavior on a session
 	if persistenceEnabled(config.Accounts.Multiclient.AutoAway, client.accountSettings.AutoAway) {
 		client.awayMessage = ""
-		if len(client.sessions) == 1 {
+		if len(newSessions) == 1 {
 			back = true
 		}
 	}
-	return true, len(client.sessions), lastSeen, back
+	return true, len(newSessions), lastSeen, back
+}
+
+func (client *Client) setSessions(sessions []*Session) {
+	// must be called while holding stateMutex.Lock()
+	sessionsPtr := new([]*Session)
+	(*sessionsPtr) = sessions
+	atomic.StorePointer(&client.sessions, unsafe.Pointer(sessionsPtr))
 }
 
 func (client *Client) removeSession(session *Session) (success bool, length int) {
-	if len(client.sessions) == 0 {
+	oldSessions := client.Sessions()
+	if len(oldSessions) == 0 {
 		return
 	}
-	sessions := make([]*Session, 0, len(client.sessions)-1)
-	for _, currentSession := range client.sessions {
+	sessions := make([]*Session, 0, len(oldSessions)-1)
+	for _, currentSession := range oldSessions {
 		if session == currentSession {
 			success = true
 		} else {
 			sessions = append(sessions, currentSession)
 		}
 	}
-	client.sessions = sessions
+	client.setSessions(sessions)
 	length = len(sessions)
 	return
 }
@@ -150,7 +162,7 @@ func (client *Client) getWhoisActually() (ip net.IP, hostname string) {
 	client.stateMutex.RLock()
 	defer client.stateMutex.RUnlock()
 
-	for _, session := range client.sessions {
+	for _, session := range client.Sessions() {
 		return session.IP(), session.rawHostname
 	}
 	return utils.IPv4LoopbackAddress, client.server.name
@@ -223,7 +235,7 @@ func (client *Client) setAutoAwayNoMutex(config *Config) {
 	// aggregate the away statuses of the individual sessions:
 	var globalAwayState string
 	var awaySetAt time.Time
-	for _, cSession := range client.sessions {
+	for _, cSession := range client.Sessions() {
 		if cSession.awayMessage == "" {
 			// a session is active, we are not auto-away
 			client.awayMessage = ""