From 28f79f57b29deba68b9e703ae91e10eefdc2004b Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Wed, 1 Dec 2021 02:11:04 -0500 Subject: [PATCH] fix #1824 Make (*Client).Sessions() lock-free --- irc/client.go | 17 ++++++++--------- irc/getters.go | 48 ++++++++++++++++++++++++++++++------------------ 2 files changed, 38 insertions(+), 27 deletions(-) diff --git a/irc/client.go b/irc/client.go index 6912483c..3e54a8df 100644 --- a/irc/client.go +++ b/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 diff --git a/irc/getters.go b/irc/getters.go index 0e49c9fc..00ad5448 100644 --- a/irc/getters.go +++ b/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 = ""