issue #104 - fix deadlock in chat sess mgr AddSession and RemoveSession

server/oscar/chat.go

@@ -71,6 +71,10 @@ func (rt ChatServer) Start(ctx context.Context) error {
 }
 
 func (rt ChatServer) handleNewConnection(ctx context.Context, rwc io.ReadWriteCloser) error {
+	defer func() {
+		rwc.Close()
+	}()
+
 	flapc := wire.NewFlapClient(100, rwc, rwc)
 	if err := flapc.SendSignonFrame(nil); err != nil {
 		return err
@@ -95,7 +99,6 @@ func (rt ChatServer) handleNewConnection(ctx context.Context, rwc io.ReadWriteCl
 
 	defer func() {
 		chatSess.Close()
-		rwc.Close()
 		rt.SignoutChat(ctx, chatSess)
 	}()
 

state/session_manager.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"log/slog"
 	"sync"
+	"time"
 
 	"github.com/mk6i/retro-aim-server/wire"
 )
@@ -201,13 +202,14 @@ type InMemoryChatSessionManager struct {
 // session is replaced by a new one.
 func (s *InMemoryChatSessionManager) AddSession(ctx context.Context, chatCookie string, screenName DisplayScreenName) (*Session, error) {
 	s.mapMutex.Lock()
-	defer s.mapMutex.Unlock()
-
 	if _, ok := s.store[chatCookie]; !ok {
 		s.store[chatCookie] = NewInMemorySessionManager(s.logger)
 	}
-
 	sessionManager := s.store[chatCookie]
+	s.mapMutex.Unlock()
+
+	ctx, cancel := context.WithTimeout(ctx, time.Second*5)
+	defer cancel()
 
 	sess, err := sessionManager.AddSession(ctx, screenName)
 	if err != nil {
@@ -216,6 +218,23 @@ func (s *InMemoryChatSessionManager) AddSession(ctx context.Context, chatCookie
 
 	sess.SetChatRoomCookie(chatCookie)
 
+	s.mapMutex.Lock()
+	defer s.mapMutex.Unlock()
+
+	// at this point it's guaranteed that the prior chat session and corresponding
+	// session manager (if the room count dropped to 0) were removed.
+	//
+	// - SessionManager.RemoveSession() was called because that unlocks
+	//   SessionManager.AddSession(), which unblocks ChatSessionManager.AddSession()
+	// - ChatSessionManager.RemoveSession() must call room deletion routine before
+	//   releasing mapMutex
+	//
+	// now restore the chat session manager, which may have been deleted by the
+	// call to RemoveSession().
+	if _, ok := s.store[chatCookie]; !ok {
+		s.store[chatCookie] = sessionManager
+	}
+
 	return sess, nil
 }
 

state/session_manager_test.go

@@ -441,22 +441,31 @@ func TestInMemoryChatSessionManager_RemoveSession(t *testing.T) {
 func TestInMemoryChatSessionManager_RemoveSession_DoubleLogin(t *testing.T) {
 	sm := NewInMemoryChatSessionManager(slog.Default())
 
-	user1, err := sm.AddSession(context.Background(), "chat-room-1", "user-screen-name-1")
+	chatSess1, err := sm.AddSession(context.Background(), "chat-room-1", "user-screen-name-1")
 	assert.NoError(t, err)
 
-	var wg sync.WaitGroup
+	wg := &sync.WaitGroup{}
 	wg.Add(1)
+
 	go func() {
-		defer wg.Done()
-		user2, err := sm.AddSession(context.Background(), "chat-room-1", "user-screen-name-1")
+		// add the session again. this call blocks until RemoveSession makes
+		// room for the new session
+		chatSess2, err := sm.AddSession(context.Background(), "chat-room-1", "user-screen-name-1")
 		assert.NoError(t, err)
-		assert.NotSame(t, user1, user2)
+		assert.Equal(t, chatSess1.DisplayScreenName(), chatSess2.DisplayScreenName())
+		wg.Done()
 	}()
 
-	sm.RemoveSession(user1)
-	wg.Wait()
+	// wait for AddSession() to block
+	for sm.mapMutex.TryRLock() {
+		sm.mapMutex.RUnlock()
+	}
+
+	// AddSession() is blocked waiting for the log. this should unblock
+	// AddSession()
+	sm.RemoveSession(chatSess1)
 
-	assert.Len(t, sm.AllSessions("chat-room-1"), 1)
+	wg.Wait()
 }
 
 func TestInMemoryChatSessionManager_RemoveUserFromAllChats(t *testing.T) {