Fix some potential race conditions (#168)

* Fix some potential race conditions

* more

* more

Author: loganmc10

internal/gameServer/server.go

@@ -148,7 +148,6 @@ func (g *GameServer) ManageBuffer() {
 				}
 			}
 		}
-		g.gameDataMutex.Unlock()
 
 		if leadPlayer > 0 {
 			if bufferHealth > float32(g.BufferTarget)+0.75 && g.gameData.bufferSize > 0 {
@@ -159,6 +158,7 @@ func (g *GameServer) ManageBuffer() {
 				g.Logger.Info("increased buffer size", "bufferHealth", bufferHealth, "bufferSize", g.gameData.bufferSize, "leadPlayer", leadPlayer)
 			}
 		}
+		g.gameDataMutex.Unlock()
 
 		time.Sleep(time.Second)
 	}
@@ -170,7 +170,9 @@ func (g *GameServer) ManagePlayers() {
 		playersActive := false // used to check if anyone is still around
 		var i byte
 
-		g.gameDataMutex.Lock() // PlayerAlive and Status can be modified by processUDP in a different thread
+		// Lock registrations before gameData so TCP paths that take registrationsMutex then gameDataMutex cannot deadlock.
+		g.registrationsMutex.Lock()
+		g.gameDataMutex.Lock()
 		for i = range 4 {
 			_, ok := g.registrations[i]
 			if ok {
@@ -181,9 +183,7 @@ func (g *GameServer) ManagePlayers() {
 					g.Logger.Info("player disconnected UDP", "player", i, "regID", g.registrations[i].regID, "address", g.gameData.playerAddresses[i])
 					g.gameData.status |= (0x1 << (i + 1))
 
-					g.registrationsMutex.Lock() // Registrations can be modified by processTCP
 					delete(g.registrations, i)
-					g.registrationsMutex.Unlock()
 
 					for k, v := range g.Players {
 						if v.Number == int(i) {
@@ -199,6 +199,7 @@ func (g *GameServer) ManagePlayers() {
 			g.gameData.playerAlive[i] = false
 		}
 		g.gameDataMutex.Unlock()
+		g.registrationsMutex.Unlock()
 
 		if !playersActive {
 			g.Logger.Info("no more players, closing room", "numPlayers", g.NumberOfPlayers, "clientSHA", g.ClientSha, "playTime", time.Since(g.StartTime).String())

internal/gameServer/tcp.go

@@ -339,13 +339,12 @@ func (g *GameServer) processTCP(conn *net.TCPConn) {
 					if v.regID == regID {
 						g.Logger.Info("player disconnected TCP", "regID", regID, "player", i, "address", conn.RemoteAddr().String())
 
-						g.gameDataMutex.Lock() // any player can modify this, which would be in a different thread
+						// Same lock order as ManagePlayers / RegisterPlayer: registrationsMutex, then gameDataMutex.
+						g.registrationsMutex.Lock()
+						g.gameDataMutex.Lock()
 						g.gameData.playerAlive[i] = false
 						g.gameData.status |= (0x1 << (i + 1))
-
-						g.registrationsMutex.Lock() // any player can modify this, which would be in a different thread
 						delete(g.registrations, i)
-						g.registrationsMutex.Unlock()
 
 						for k, v := range g.Players {
 							if v.Number == int(i) {
@@ -357,6 +356,7 @@ func (g *GameServer) processTCP(conn *net.TCPConn) {
 						}
 						g.gameData.bufferHealth[i] = g.gameData.bufferHealth[i][:0]
 						g.gameDataMutex.Unlock()
+						g.registrationsMutex.Unlock()
 					}
 				}
 			}

internal/gameServer/udp.go

@@ -125,6 +125,7 @@ func (g *GameServer) processUDP(addr *net.UDPAddr) {
 	playerNumber := g.gameData.recvBuffer[1]
 	switch g.gameData.recvBuffer[0] {
 	case KeyInfoClient:
+		g.gameDataMutex.Lock()
 		g.gameData.playerAddresses[playerNumber] = addr
 		count := binary.BigEndian.Uint32(g.gameData.recvBuffer[2:])
 
@@ -138,19 +139,20 @@ func (g *GameServer) processUDP(addr *net.UDPAddr) {
 				g.sendUDPInput(count, g.gameData.playerAddresses[i], playerNumber, true, NoRegID)
 			}
 		}
+		g.gameDataMutex.Unlock()
 	case PlayerInputRequest:
 		regID := binary.BigEndian.Uint32(g.gameData.recvBuffer[2:])
 		count := binary.BigEndian.Uint32(g.gameData.recvBuffer[6:])
 		spectator := g.gameData.recvBuffer[10]
-		if uintLarger(count, g.gameData.leadCount) && spectator == 0 {
-			g.gameData.leadCount = count
-		}
 		sendingPlayerNumber, err := g.getPlayerNumberByID(regID)
 		if err != nil {
 			g.Logger.Error(err, "could not process request", "regID", regID)
 			return
 		}
-		g.gameDataMutex.Lock() // playerAlive, countLag and bufferHealth can be modified in different threads
+		g.gameDataMutex.Lock() // playerAlive, countLag, bufferHealth, leadCount; other threads touch these
+		if uintLarger(count, g.gameData.leadCount) && spectator == 0 {
+			g.gameData.leadCount = count
+		}
 		g.gameData.countLag[sendingPlayerNumber] = append(g.gameData.countLag[sendingPlayerNumber], g.sendUDPInput(count, addr, playerNumber, spectator != 0, sendingPlayerNumber))
 		g.gameData.bufferHealth[sendingPlayerNumber] = append(g.gameData.bufferHealth[sendingPlayerNumber], g.gameData.recvBuffer[11])
 		g.gameData.playerAlive[sendingPlayerNumber] = true

internal/lobbyServer/lobby.go

@@ -198,6 +198,7 @@ func (s *LobbyServer) publishDiscord(message string, channel string) {
 	httpRequest, err := retryablehttp.NewRequest(http.MethodPost, channel, bodyJSON)
 	if err != nil {
 		s.Logger.Error(err, "could not create request")
+		return
 	}
 	httpRequest.Header.Set("Content-Type", "application/json")
 	httpRequest.Header.Set("User-Agent", "gopher64Bot (gopher64.github.io, 1)")