Lock on AgentCircuitData during Scene.AddClient() and RemoveClient() to prevent an inactive connection being left behind if the user closes the viewer whilst the connection is being established.

This should remove the need to run the console command "kick user --force" when these connections are left around.

OpenSim/Region/ClientStack/Linden/Caps/EventQueue/EventQueueGetModule.cs

@@ -94,7 +94,7 @@ namespace OpenSim.Region.ClientStack.Linden
                 
                 //scene.CommsManager.HttpServer.AddLLSDHandler("/CAPS/EQG/", EventQueueFallBack);
 
-                scene.EventManager.OnNewClient += OnNewClient;
+//                scene.EventManager.OnNewClient += OnNewClient;
 
                 // TODO: Leaving these open, or closing them when we
                 // become a child is incorrect. It messes up TP in a big
@@ -102,6 +102,7 @@ namespace OpenSim.Region.ClientStack.Linden
                 // circuit is there.
 
                 scene.EventManager.OnClientClosed += ClientClosed;
+                
                 scene.EventManager.OnMakeChildAgent += MakeChildAgent;
                 scene.EventManager.OnRegisterCaps += OnRegisterCaps;
 
@@ -226,16 +227,6 @@ namespace OpenSim.Region.ClientStack.Linden
 
         #endregion
 
-        private void OnNewClient(IClientAPI client)
-        {
-            //client.OnLogout += ClientClosed;
-        }
-
-//        private void ClientClosed(IClientAPI client)
-//        {
-//            ClientClosed(client.AgentId);
-//        }
-
         private void ClientClosed(UUID agentID, Scene scene)
         {
 //            m_log.DebugFormat("[EVENTQUEUE]: Closed client {0} in region {1}", agentID, m_scene.RegionInfo.RegionName);

OpenSim/Region/ClientStack/Linden/UDP/LLUDPServer.cs

@@ -1103,9 +1103,9 @@ namespace OpenSim.Region.ClientStack.LindenUDP
         {
             IClientAPI client = null;
 
-            // In priciple there shouldn't be more than one thread here, ever.
+            // We currently synchronize this code across the whole scene to avoid issues such as
-            // But in case that happens, we need to synchronize this piece of code
+            // http://opensimulator.org/mantis/view.php?id=5365  However, once locking per agent circuit can be done
-            // because it's too important
+            // consistently, this lock could probably be removed.
             lock (this)
             {
                 if (!m_scene.TryGetClient(agentID, out client))

OpenSim/Region/Framework/Scenes/Scene.cs

@@ -79,6 +79,11 @@ namespace OpenSim.Region.Framework.Scenes
 
         public SynchronizeSceneHandler SynchronizeScene;
 
+        /// <summary>
+        /// Used to prevent simultaneous calls to RemoveClient() for the same agent from interfering with each other.
+        /// </summary>
+        private object m_removeClientLock = new object();
+
         /// <summary>
         /// Statistical information for this scene.
         /// </summary>
@@ -2709,16 +2714,35 @@ namespace OpenSim.Region.Framework.Scenes
 
         public override ISceneAgent AddNewClient(IClientAPI client, PresenceType type)
         {
+            ScenePresence sp;
+            bool vialogin;
+
             // Validation occurs in LLUDPServer
+            //
+            // XXX: A race condition exists here where two simultaneous calls to AddNewClient can interfere with
+            // each other.  In practice, this does not currently occur in the code.
             AgentCircuitData aCircuit = m_authenticateHandler.GetAgentCircuitData(client.CircuitCode);
 
-            bool vialogin
+            // We lock here on AgentCircuitData to prevent a race condition between the thread adding a new connection
+            // and a simultaneous one that removes it (as can happen if the client is closed at a particular point
+            // whilst connecting).
+            //
+            // It would be easier to lock across all NewUserConnection(), AddNewClient() and
+            // RemoveClient() calls for all agents, but this would allow a slow call (e.g. because of slow service
+            // response in some module listening to AddNewClient()) from holding up unrelated agent calls.
+            //
+            // In practice, the lock (this) in LLUDPServer.AddNewClient() currently lock across all
+            // AddNewClient() operations (though not other ops).
+            // In the future this can be relieved once locking per agent (not necessarily on AgentCircuitData) is improved.
+            lock (aCircuit)
+            {
+                vialogin
                     = (aCircuit.teleportFlags & (uint)Constants.TeleportFlags.ViaHGLogin) != 0
                         || (aCircuit.teleportFlags & (uint)Constants.TeleportFlags.ViaLogin) != 0;
     
     //            CheckHeartbeat();
     
-            ScenePresence sp = GetScenePresence(client.AgentId);
+                sp = GetScenePresence(client.AgentId);
 
                 // XXX: Not sure how good it is to add a new client if a scene presence already exists.  Possibly this
                 // could occur if a viewer crashes and relogs before the old client is kicked out.  But this could cause
@@ -2763,14 +2787,15 @@ namespace OpenSim.Region.Framework.Scenes
                 // client is for a root or child agent.
                 client.SceneAgent = sp;
 
-            m_LastLogin = Util.EnvironmentTickCount();
-
                 // Cache the user's name
                 CacheUserName(sp, aCircuit);
     
                 EventManager.TriggerOnNewClient(client);
                 if (vialogin)
                     EventManager.TriggerOnClientLogin(client);
+            }
+
+            m_LastLogin = Util.EnvironmentTickCount();
 
             return sp;
         }
@@ -3300,6 +3325,27 @@ namespace OpenSim.Region.Framework.Scenes
         {
 //            CheckHeartbeat();
             bool isChildAgent = false;
+            AgentCircuitData acd;
+
+            lock (m_removeClientLock)
+            {
+                acd = m_authenticateHandler.GetAgentCircuitData(agentID);
+
+                if (acd == null)
+                {
+                    m_log.ErrorFormat("[SCENE]: No agent circuit found for {0}, aborting Scene.RemoveClient", agentID);
+                    return;
+                }
+                else
+                {
+                    // We remove the acd up here to avoid later raec conditions if two RemoveClient() calls occurred
+                    // simultaneously.
+                    m_authenticateHandler.RemoveCircuit(acd.circuitcode);
+                }
+            }
+
+            lock (acd)
+            {
                 ScenePresence avatar = GetScenePresence(agentID);
     
                 if (avatar == null)
@@ -3379,8 +3425,6 @@ namespace OpenSim.Region.Framework.Scenes
                     // It's possible for child agents to have transactions if changes are being made cross-border.
                     if (AgentTransactionsModule != null)
                         AgentTransactionsModule.RemoveAgentAssetTransactions(agentID);
-
-                m_authenticateHandler.RemoveCircuit(avatar.ControllingClient.CircuitCode);
                 }
                 catch (Exception e)
                 {
@@ -3405,6 +3449,7 @@ namespace OpenSim.Region.Framework.Scenes
                             string.Format("[SCENE]: Exception in final clean up of {0} in {1}.  Exception ", avatar.Name, Name), e);
                     }
                 }
+            }
 
             //m_log.InfoFormat("[SCENE] Memory pre  GC {0}", System.GC.GetTotalMemory(false));
             //m_log.InfoFormat("[SCENE] Memory post GC {0}", System.GC.GetTotalMemory(true));
@@ -3574,6 +3619,10 @@ namespace OpenSim.Region.Framework.Scenes
                 return false;
             }
 
+            ILandObject land;
+
+            lock (agent)
+            {
                 ScenePresence sp = GetScenePresence(agent.AgentID);
     
                 if (sp != null && !sp.IsChildAgent)
@@ -3589,7 +3638,7 @@ namespace OpenSim.Region.Framework.Scenes
                     sp = null;
                 }
     
-            ILandObject land = LandChannel.GetLandObject(agent.startpos.X, agent.startpos.Y);
+                land = LandChannel.GetLandObject(agent.startpos.X, agent.startpos.Y);
     
                 //On login test land permisions
                 if (vialogin)
@@ -3608,10 +3657,12 @@ namespace OpenSim.Region.Framework.Scenes
                         {
                             if (!VerifyUserPresence(agent, out reason))
                                 return false;
-                    } catch (Exception e)
+                        }
+                        catch (Exception e)
                         {
                             m_log.ErrorFormat(
                                 "[SCENE]: Exception verifying presence {0}{1}", e.Message, e.StackTrace);
+
                             return false;
                         }
                     }
@@ -3620,10 +3671,12 @@ namespace OpenSim.Region.Framework.Scenes
                     {
                         if (!AuthorizeUser(agent, out reason))
                             return false;
-                } catch (Exception e)
+                    }
+                    catch (Exception e)
                     {
                         m_log.ErrorFormat(
                             "[SCENE]: Exception authorizing user {0}{1}", e.Message, e.StackTrace);
+
                         return false;
                     }
     
@@ -3637,7 +3690,8 @@ namespace OpenSim.Region.Framework.Scenes
                         CapsModule.SetAgentCapsSeeds(agent);
                         CapsModule.CreateCaps(agent.AgentID);
                     }
-            } else
+                }
+                else
                 {
                     // Let the SP know how we got here. This has a lot of interesting
                     // uses down the line.
@@ -3655,6 +3709,7 @@ namespace OpenSim.Region.Framework.Scenes
                             CapsModule.SetAgentCapsSeeds(agent);
                     }
                 }
+            }
 
             // In all cases, add or update the circuit data with the new agent circuit data and teleport flags
             agent.teleportFlags = teleportFlags;