Prevent race conditions between two threads that call LLClientView.Close() simultaneously (e.g. ack timeout and an attempt to reconnect)

3 files changed, 60 insertions, 33 deletions

diff --git a/OpenSim/Region/ClientStack/Linden/UDP/LLClientView.cs b/OpenSim/Region/ClientStack/Linden/UDP/LLClientView.cs
index 73cdec3..ae5207b 100644
--- a/OpenSim/Region/ClientStack/Linden/UDP/LLClientView.cs
+++ b/OpenSim/Region/ClientStack/Linden/UDP/LLClientView.cs
@@ -347,8 +347,6 @@ namespace OpenSim.Region.ClientStack.LindenUDP
         private int m_animationSequenceNumber = 1;
         private bool m_SendLogoutPacketWhenClosing = true;
         private AgentUpdateArgs lastarg;
-        private bool m_IsActive = true;
-        private bool m_IsLoggingOut = false;
 
         protected Dictionary<PacketType, PacketProcessor> m_packetHandlers = new Dictionary<PacketType, PacketProcessor>();
         protected Dictionary<string, GenericMessage> m_genericPacketHandlers = new Dictionary<string, GenericMessage>(); //PauPaw:Local Generic Message handlers
@@ -412,16 +410,19 @@ namespace OpenSim.Region.ClientStack.LindenUDP
         public uint CircuitCode { get { return m_circuitCode; } }
         public int MoneyBalance { get { return m_moneyBalance; } }
         public int NextAnimationSequenceNumber { get { return m_animationSequenceNumber++; } }
-        public bool IsActive
-        {
-            get { return m_IsActive; }
-            set { m_IsActive = value; }
-        }
-        public bool IsLoggingOut
-        {
-            get { return m_IsLoggingOut; }
-            set { m_IsLoggingOut = value; }
-        }
+
+        /// <summary>
+        /// As well as it's function in IClientAPI, in LLClientView we are locking on this property in order to
+        /// prevent race conditions by different threads calling Close().
+        /// </summary>
+        public bool IsActive { get; set; }
+
+        /// <summary>
+        /// Used to synchronise threads when client is being closed.
+        /// </summary>
+        public Object CloseSyncLock { get; private set; }
+
+        public bool IsLoggingOut { get; set; }
 
         public bool DisableFacelights
         {
@@ -446,6 +447,8 @@ namespace OpenSim.Region.ClientStack.LindenUDP
         {
 //            DebugPacketLevel = 1;
 
+            CloseSyncLock = new Object();
+
             RegisterInterface<IClientIM>(this);
             RegisterInterface<IClientInventory>(this);
             RegisterInterface<IClientChat>(this);
@@ -478,17 +481,40 @@ namespace OpenSim.Region.ClientStack.LindenUDP
             m_prioritizer = new Prioritizer(m_scene);
 
             RegisterLocalPacketHandlers();
+
+            IsActive = true;
         }
 
         #region Client Methods
 
         /// <summary>
-        /// Shut down the client view
+        /// Close down the client view
         /// </summary>
         public void Close()
         {
-            IsActive = false;
+            // We lock here to prevent race conditions between two threads calling close simultaneously (e.g.
+            // a simultaneous relog just as a client is being closed out due to no packet ack from the old connection.
+            lock (CloseSyncLock)
+            {
+                if (!IsActive)
+                    return;
+
+                IsActive = false;
+                CloseWithoutChecks();
+            }
+        }
 
+        /// <summary>
+        /// Closes down the client view without first checking whether it is active.
+        /// </summary>
+        /// <remarks>
+        /// This exists because LLUDPServer has to set IsActive = false in earlier synchronous code before calling
+        /// CloseWithoutIsActiveCheck asynchronously.
+        ///
+        /// Callers must lock ClosingSyncLock before calling.
+        /// </remarks>
+        public void CloseWithoutChecks()
+        {
             m_log.DebugFormat(
                 "[CLIENT]: Close has been called for {0} attached to scene {1}",
                 Name, m_scene.RegionInfo.RegionName);
@@ -3567,7 +3593,9 @@ namespace OpenSim.Region.ClientStack.LindenUDP
 
         public void SendCoarseLocationUpdate(List<UUID> users, List<Vector3> CoarseLocations)
         {
-            if (!IsActive) return; // We don't need to update inactive clients.
+            // We don't need to update inactive clients.
+            if (!IsActive)
+                return;
 
             CoarseLocationUpdatePacket loc = (CoarseLocationUpdatePacket)PacketPool.Instance.GetPacket(PacketType.CoarseLocationUpdate);
             loc.Header.Reliable = false;
diff --git a/OpenSim/Region/ClientStack/Linden/UDP/LLUDPServer.cs b/OpenSim/Region/ClientStack/Linden/UDP/LLUDPServer.cs
index 097f109..746eb90 100644
--- a/OpenSim/Region/ClientStack/Linden/UDP/LLUDPServer.cs
+++ b/OpenSim/Region/ClientStack/Linden/UDP/LLUDPServer.cs
@@ -1123,22 +1123,21 @@ namespace OpenSim.Region.ClientStack.LindenUDP
         /// regular client pings.
         /// </remarks>
         /// <param name='client'></param>
-        private void DeactivateClientDueToTimeout(IClientAPI client)
+        private void DeactivateClientDueToTimeout(LLClientView client)
         {
-            // We must set IsActive synchronously so that we can stop the packet loop reinvoking this method, even
-            // though it's set later on by LLClientView.Close()
-            client.IsActive = false;
-
-            m_log.WarnFormat(
-                "[LLUDPSERVER]: Ack timeout, disconnecting {0} agent for {1} in {2}",
-                client.SceneAgent.IsChildAgent ? "child" : "root", client.Name, m_scene.RegionInfo.RegionName);
-
-            StatsManager.SimExtraStats.AddAbnormalClientThreadTermination();
-
-            if (!client.SceneAgent.IsChildAgent)
-                 client.Kick("Simulator logged you out due to connection timeout");
-
-            client.Close();
+            lock (client.CloseSyncLock)
+            {
+                m_log.WarnFormat(
+                    "[LLUDPSERVER]: Ack timeout, disconnecting {0} agent for {1} in {2}",
+                    client.SceneAgent.IsChildAgent ? "child" : "root", client.Name, m_scene.RegionInfo.RegionName);
+    
+                StatsManager.SimExtraStats.AddAbnormalClientThreadTermination();
+    
+                if (!client.SceneAgent.IsChildAgent)
+                     client.Kick("Simulator logged you out due to connection timeout");
+    
+                client.CloseWithoutChecks();
+            }
         }
 
         private void IncomingPacketHandler()
diff --git a/OpenSim/Region/Framework/Scenes/Scene.cs b/OpenSim/Region/Framework/Scenes/Scene.cs
index 36452de..51a6820 100644
--- a/OpenSim/Region/Framework/Scenes/Scene.cs
+++ b/OpenSim/Region/Framework/Scenes/Scene.cs
@@ -3517,8 +3517,8 @@ namespace OpenSim.Region.Framework.Scenes
                 // We have a zombie from a crashed session. 
                 // Or the same user is trying to be root twice here, won't work.
                 // Kill it.
-                m_log.DebugFormat(
-                    "[SCENE]: Zombie scene presence detected for {0} {1} in {2}",
+                m_log.WarnFormat(
+                    "[SCENE]: Existing root scene presence detected for {0} {1} in {2} when connecting.  Removing existing presence.",
                     sp.Name, sp.UUID, RegionInfo.RegionName);
 
                 sp.ControllingClient.Close();
@@ -4474,7 +4474,7 @@ namespace OpenSim.Region.Framework.Scenes
         /// </summary>
         /// <remarks>
         /// This method will return both root and child scene presences.
-        /// 
+        ///
         /// Consider using ForEachScenePresence() or ForEachRootScenePresence() if possible since these will not
         /// involving creating a new List object.
         /// </remarks>