fix several session destruction bugs

irc/client.go

@@ -323,8 +323,10 @@ func (client *Client) run(session *Session) {
 	session.resetFakelag()
 
 	isReattach := client.Registered()
-	// don't reset the nick timer during a reattach
-	if !isReattach {
+	if isReattach {
+		client.playReattachMessages(session)
+	} else {
+		// don't reset the nick timer during a reattach
 		client.nickTimer.Initialize(client)
 	}
 
@@ -386,14 +388,14 @@ func (client *Client) run(session *Session) {
 			break
 		} else if session.client != client {
 			// bouncer reattach
-			session.playReattachMessages()
 			go session.client.run(session)
 			break
 		}
 	}
 }
 
-func (session *Session) playReattachMessages() {
+func (client *Client) playReattachMessages(session *Session) {
+	client.server.playRegistrationBurst(session)
 	for _, channel := range session.client.Channels() {
 		channel.playJoinForSession(session)
 	}
@@ -922,14 +924,11 @@ func (client *Client) destroy(beingResumed bool, session *Session) {
 
 	// allow destroy() to execute at most once
 	client.stateMutex.Lock()
-	nickMaskString := client.nickMaskString
-	accountName := client.accountName
-
-	alreadyDestroyed := len(client.sessions) == 0
+	details := client.detailsNoMutex()
+	wasReattach := session != nil && session.client != client
 	sessionRemoved := false
 	var remainingSessions int
 	if session == nil {
-		sessionRemoved = !alreadyDestroyed
 		sessionsToDestroy = client.sessions
 		client.sessions = nil
 		remainingSessions = 0
@@ -939,33 +938,46 @@ func (client *Client) destroy(beingResumed bool, session *Session) {
 			sessionsToDestroy = []*Session{session}
 		}
 	}
-	var quitMessage string
-	if 0 < len(sessionsToDestroy) {
-		quitMessage = sessionsToDestroy[0].quitMessage
-	}
 	client.stateMutex.Unlock()
 
-	if alreadyDestroyed || !sessionRemoved {
+	if len(sessionsToDestroy) == 0 {
 		return
 	}
 
+	// destroy all applicable sessions:
+	var quitMessage string
 	for _, session := range sessionsToDestroy {
 		if session.client != client {
 			// session has been attached to a new client; do not destroy it
 			continue
 		}
 		session.idletimer.Stop()
-		session.socket.Close()
 		// send quit/error message to client if they haven't been sent already
 		client.Quit("", session)
+		quitMessage = session.quitMessage
+		session.socket.Close()
+
+		// remove from connection limits
+		var source string
+		if client.isTor {
+			client.server.torLimiter.RemoveClient()
+			source = "tor"
+		} else {
+			ip := session.realIP
+			if session.proxiedIP != nil {
+				ip = session.proxiedIP
+			}
+			client.server.connectionLimiter.RemoveClient(ip)
+			source = ip.String()
+		}
+		client.server.logger.Info("localconnect-ip", fmt.Sprintf("disconnecting session of %s from %s", details.nick, source))
 	}
 
+	// ok, now destroy the client, unless it still has sessions:
 	if remainingSessions != 0 {
 		return
 	}
 
-	details := client.Details()
-
 	// see #235: deduplicating the list of PART recipients uses (comparatively speaking)
 	// a lot of RAM, so limit concurrency to avoid thrashing
 	client.server.semaphores.ClientDestroy.Acquire()
@@ -973,38 +985,35 @@ func (client *Client) destroy(beingResumed bool, session *Session) {
 
 	if beingResumed {
 		client.server.logger.Debug("quit", fmt.Sprintf("%s is being resumed", details.nick))
-	} else {
+	} else if !wasReattach {
 		client.server.logger.Debug("quit", fmt.Sprintf("%s is no longer on the server", details.nick))
 	}
 
-	if !beingResumed {
-		client.server.whoWas.Append(details.WhoWas)
-	}
-
-	// remove from connection limits
-	if client.isTor {
-		client.server.torLimiter.RemoveClient()
-	} else {
-		client.server.connectionLimiter.RemoveClient(client.IP())
+	registered := client.Registered()
+	if !beingResumed && registered {
+		client.server.whoWas.Append(client.WhoWas())
 	}
 
 	client.server.resumeManager.Delete(client)
 
 	// alert monitors
-	client.server.monitorManager.AlertAbout(client, false)
+	if registered {
+		client.server.monitorManager.AlertAbout(client, false)
+	}
 	// clean up monitor state
 	client.server.monitorManager.RemoveAll(client)
 
 	splitQuitMessage := utils.MakeSplitMessage(quitMessage, true)
 	// clean up channels
+	// (note that if this is a reattach, client has no channels and therefore no friends)
 	friends := make(ClientSet)
 	for _, channel := range client.Channels() {
 		if !beingResumed {
 			channel.Quit(client)
 			channel.history.Add(history.Item{
 				Type:        history.Quit,
-				Nick:        nickMaskString,
-				AccountName: accountName,
+				Nick:        details.nickMask,
+				AccountName: details.accountName,
 				Message:     splitQuitMessage,
 			})
 		}

irc/client_lookup_set.go

@@ -160,9 +160,7 @@ func (clients *ClientManager) SetNick(client *Client, session *Session, newNick
 		if !currentClient.AddSession(session) {
 			return errNicknameInUse
 		}
-		// successful reattach. temporarily assign them the nick they'll have going forward
-		// (the current `client` will be discarded at the end of command execution)
-		client.updateNick(currentClient.Nick(), newcfnick, newSkeleton)
+		// successful reattach!
 		return nil
 	}
 	// analogous checks for skeletons

irc/getters.go

@@ -329,7 +329,10 @@ func (client *Client) WhoWas() (result WhoWas) {
 func (client *Client) Details() (result ClientDetails) {
 	client.stateMutex.RLock()
 	defer client.stateMutex.RUnlock()
+	return client.detailsNoMutex()
+}
 
+func (client *Client) detailsNoMutex() (result ClientDetails) {
 	result.nick = client.nick
 	result.nickCasefolded = client.nickCasefolded
 	result.username = client.username

irc/server.go

@@ -413,20 +413,30 @@ func (server *Server) tryRegister(c *Client, session *Session) {
 		}
 	}
 
-	reattached := session.client != c
-
-	if !reattached {
-		// registration has succeeded:
-		c.SetRegistered()
-
-		// count new user in statistics
-		server.stats.ChangeTotal(1)
-
-		if !resumed {
-			server.monitorManager.AlertAbout(c, true)
-		}
+	if session.client != c {
+		// reattached, bail out.
+		// we'll play the reg burst later, on the new goroutine associated with
+		// (thisSession, otherClient). This is to avoid having to transfer state
+		// like nickname, hostname, etc. to show the correct values in the reg burst.
+		return
 	}
 
+	// registration has succeeded:
+	c.SetRegistered()
+	// count new user in statistics
+	server.stats.ChangeTotal(1)
+
+	server.playRegistrationBurst(session)
+
+	if resumed {
+		c.tryResumeChannels()
+	} else {
+		server.monitorManager.AlertAbout(c, true)
+	}
+}
+
+func (server *Server) playRegistrationBurst(session *Session) {
+	c := session.client
 	// continue registration
 	d := c.Details()
 	server.logger.Info("localconnect", fmt.Sprintf("Client connected [%s] [u:%s] [r:%s]", d.nick, d.username, d.realname))
@@ -435,11 +445,11 @@ func (server *Server) tryRegister(c *Client, session *Session) {
 	// send welcome text
 	//NOTE(dan): we specifically use the NICK here instead of the nickmask
 	// see http://modern.ircdocs.horse/#rplwelcome-001 for details on why we avoid using the nickmask
-	c.Send(nil, server.name, RPL_WELCOME, d.nick, fmt.Sprintf(c.t("Welcome to the Internet Relay Network %s"), d.nick))
-	c.Send(nil, server.name, RPL_YOURHOST, d.nick, fmt.Sprintf(c.t("Your host is %[1]s, running version %[2]s"), server.name, Ver))
-	c.Send(nil, server.name, RPL_CREATED, d.nick, fmt.Sprintf(c.t("This server was created %s"), server.ctime.Format(time.RFC1123)))
+	session.Send(nil, server.name, RPL_WELCOME, d.nick, fmt.Sprintf(c.t("Welcome to the Internet Relay Network %s"), d.nick))
+	session.Send(nil, server.name, RPL_YOURHOST, d.nick, fmt.Sprintf(c.t("Your host is %[1]s, running version %[2]s"), server.name, Ver))
+	session.Send(nil, server.name, RPL_CREATED, d.nick, fmt.Sprintf(c.t("This server was created %s"), server.ctime.Format(time.RFC1123)))
 	//TODO(dan): Look at adding last optional [<channel modes with a parameter>] parameter
-	c.Send(nil, server.name, RPL_MYINFO, d.nick, server.name, Ver, supportedUserModesString, supportedChannelModesString)
+	session.Send(nil, server.name, RPL_MYINFO, d.nick, server.name, Ver, supportedUserModesString, supportedChannelModesString)
 
 	rb := NewResponseBuffer(session)
 	c.RplISupport(rb)
@@ -448,14 +458,10 @@ func (server *Server) tryRegister(c *Client, session *Session) {
 
 	modestring := c.ModeString()
 	if modestring != "+" {
-		c.Send(nil, d.nickMask, RPL_UMODEIS, d.nick, c.ModeString())
+		session.Send(nil, d.nickMask, RPL_UMODEIS, d.nick, modestring)
 	}
 	if server.logger.IsLoggingRawIO() {
-		c.Notice(c.t("This server is in debug mode and is logging all user I/O. If you do not wish for everything you send to be readable by the server owner(s), please disconnect."))
-	}
-
-	if resumed {
-		c.tryResumeChannels()
+		session.Send(nil, c.server.name, "NOTICE", d.nick, c.t("This server is in debug mode and is logging all user I/O. If you do not wish for everything you send to be readable by the server owner(s), please disconnect."))
 	}
 }