3 files changed, 265 insertions, 219 deletions

diff --git a/OpenSim/Region/ClientStack/Linden/Caps/EventQueue/EventQueueGetModule.cs b/OpenSim/Region/ClientStack/Linden/Caps/EventQueue/EventQueueGetModule.cs
index 594b229..0dd0904 100644
--- a/OpenSim/Region/ClientStack/Linden/Caps/EventQueue/EventQueueGetModule.cs
+++ b/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);
diff --git a/OpenSim/Region/ClientStack/Linden/UDP/LLUDPServer.cs b/OpenSim/Region/ClientStack/Linden/UDP/LLUDPServer.cs
index d11fcbf..ab670a7 100644
--- a/OpenSim/Region/ClientStack/Linden/UDP/LLUDPServer.cs
+++ b/OpenSim/Region/ClientStack/Linden/UDP/LLUDPServer.cs
@@ -1103,20 +1103,20 @@ namespace OpenSim.Region.ClientStack.LindenUDP
         {
             IClientAPI client = null;
 
-            // In priciple there shouldn't be more than one thread here, ever.
-            // But in case that happens, we need to synchronize this piece of code
-            // because it's too important
-            lock (this) 
+            // We currently synchronize this code across the whole scene to avoid issues such as
+            // http://opensimulator.org/mantis/view.php?id=5365  However, once locking per agent circuit can be done
+            // consistently, this lock could probably be removed.
+            lock (this)
             {
                 if (!m_scene.TryGetClient(agentID, out client))
                 {
                     LLUDPClient udpClient = new LLUDPClient(this, ThrottleRates, m_throttle, circuitCode, agentID, remoteEndPoint, m_defaultRTO, m_maxRTO);
-
+    
                     client = new LLClientView(m_scene, this, udpClient, sessionInfo, agentID, sessionID, circuitCode);
                     client.OnLogout += LogoutHandler;
-
+    
                     ((LLClientView)client).DisableFacelights = m_disableFacelights;
-
+    
                     client.Start();
                 }
             }
diff --git a/OpenSim/Region/Framework/Scenes/Scene.cs b/OpenSim/Region/Framework/Scenes/Scene.cs
index ed88571..fb2decc 100644
--- a/OpenSim/Region/Framework/Scenes/Scene.cs
+++ b/OpenSim/Region/Framework/Scenes/Scene.cs
@@ -80,6 +80,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>
         public SimStatsReporter StatsReporter { get; private set; }
@@ -2709,69 +2714,89 @@ 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
-                = (aCircuit.teleportFlags & (uint)Constants.TeleportFlags.ViaHGLogin) != 0
-                    || (aCircuit.teleportFlags & (uint)Constants.TeleportFlags.ViaLogin) != 0;
-
-//            CheckHeartbeat();
-
-            ScenePresence 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
-            // other problems, and possible the code calling AddNewClient() should ensure that no client is already
-            // connected.
-            if (sp == null)
-            {
-                m_log.DebugFormat(
-                    "[SCENE]: Adding new child scene presence {0} {1} to scene {2} at pos {3}",
-                    client.Name, client.AgentId, RegionInfo.RegionName, client.StartPos);
-
-                m_clientManager.Add(client);
-                SubscribeToClientEvents(client);
-
-                sp = m_sceneGraph.CreateAndAddChildScenePresence(client, aCircuit.Appearance, type);
-                m_eventManager.TriggerOnNewPresence(sp);
-
-                sp.TeleportFlags = (TPFlags)aCircuit.teleportFlags;
+            // 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();
+    
+                sp = GetScenePresence(client.AgentId);
 
-                // The first agent upon login is a root agent by design.
-                // For this agent we will have to rez the attachments.
-                // All other AddNewClient calls find aCircuit.child to be true.
-                if (aCircuit.child == false)
+                // 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
+                // other problems, and possible the code calling AddNewClient() should ensure that no client is already
+                // connected.
+                if (sp == null)
                 {
-                    // We have to set SP to be a root agent here so that SP.MakeRootAgent() will later not try to
-                    // start the scripts again (since this is done in RezAttachments()).
-                    // XXX: This is convoluted.
-                    sp.IsChildAgent = false;
-
-                    if (AttachmentsModule != null)
-                        Util.FireAndForget(delegate(object o) { AttachmentsModule.RezAttachments(sp); });
+                    m_log.DebugFormat(
+                        "[SCENE]: Adding new child scene presence {0} {1} to scene {2} at pos {3}",
+                        client.Name, client.AgentId, RegionInfo.RegionName, client.StartPos);
+    
+                    m_clientManager.Add(client);
+                    SubscribeToClientEvents(client);
+    
+                    sp = m_sceneGraph.CreateAndAddChildScenePresence(client, aCircuit.Appearance, type);
+                    m_eventManager.TriggerOnNewPresence(sp);
+    
+                    sp.TeleportFlags = (TPFlags)aCircuit.teleportFlags;
+    
+                    // The first agent upon login is a root agent by design.
+                    // For this agent we will have to rez the attachments.
+                    // All other AddNewClient calls find aCircuit.child to be true.
+                    if (aCircuit.child == false)
+                    {
+                        // We have to set SP to be a root agent here so that SP.MakeRootAgent() will later not try to
+                        // start the scripts again (since this is done in RezAttachments()).
+                        // XXX: This is convoluted.
+                        sp.IsChildAgent = false;
+    
+                        if (AttachmentsModule != null)
+                            Util.FireAndForget(delegate(object o) { AttachmentsModule.RezAttachments(sp); });
+                    }
                 }
-            }
-            else
-            {
-                m_log.WarnFormat(
-                    "[SCENE]: Already found {0} scene presence for {1} in {2} when asked to add new scene presence",
-                    sp.IsChildAgent ? "child" : "root", sp.Name, RegionInfo.RegionName);
-            }
+                else
+                {
+                    m_log.WarnFormat(
+                        "[SCENE]: Already found {0} scene presence for {1} in {2} when asked to add new scene presence",
+                        sp.IsChildAgent ? "child" : "root", sp.Name, RegionInfo.RegionName);
+                }
+    
+                // We must set this here so that TriggerOnNewClient and TriggerOnClientLogin can determine whether the
+                // client is for a root or child agent.
+                client.SceneAgent = sp;
 
-            // We must set this here so that TriggerOnNewClient and TriggerOnClientLogin can determine whether the
-            // client is for a root or child agent.
-            client.SceneAgent = sp;
+                // Cache the user's name
+                CacheUserName(sp, aCircuit);
+    
+                EventManager.TriggerOnNewClient(client);
+                if (vialogin)
+                    EventManager.TriggerOnClientLogin(client);
+            }
 
             m_LastLogin = Util.EnvironmentTickCount();
 
-            // Cache the user's name
-            CacheUserName(sp, aCircuit);
-
-            EventManager.TriggerOnNewClient(client);
-            if (vialogin)
-                EventManager.TriggerOnClientLogin(client);
-
             return sp;
         }
 
@@ -3300,109 +3325,129 @@ namespace OpenSim.Region.Framework.Scenes
         {
 //            CheckHeartbeat();
             bool isChildAgent = false;
-            ScenePresence avatar = GetScenePresence(agentID);
-
-            if (avatar == null)
-            {
-                m_log.WarnFormat(
-                    "[SCENE]: Called RemoveClient() with agent ID {0} but no such presence is in the scene.", agentID);
-
-                return;
-            }
+            AgentCircuitData acd;
 
-            try
+            lock (m_removeClientLock)
             {
-                isChildAgent = avatar.IsChildAgent;
-
-                m_log.DebugFormat(
-                    "[SCENE]: Removing {0} agent {1} {2} from {3}",
-                    (isChildAgent ? "child" : "root"), avatar.Name, agentID, RegionInfo.RegionName);
+                acd = m_authenticateHandler.GetAgentCircuitData(agentID);
 
-                // Don't do this to root agents, it's not nice for the viewer
-                if (closeChildAgents && isChildAgent)
+                if (acd == null)
                 {
-                    // Tell a single agent to disconnect from the region.
-                    IEventQueue eq = RequestModuleInterface<IEventQueue>();
-                    if (eq != null)
-                    {
-                        eq.DisableSimulator(RegionInfo.RegionHandle, avatar.UUID);
-                    }
-                    else
-                    {
-                        avatar.ControllingClient.SendShutdownConnectionNotice();
-                    }
+                    m_log.ErrorFormat("[SCENE]: No agent circuit found for {0}, aborting Scene.RemoveClient", agentID);
+                    return;
                 }
-
-                // Only applies to root agents.
-                if (avatar.ParentID != 0)
+                else
                 {
-                    avatar.StandUp();
+                    // We remove the acd up here to avoid later raec conditions if two RemoveClient() calls occurred
+                    // simultaneously.
+                    m_authenticateHandler.RemoveCircuit(acd.circuitcode);
                 }
+            }
 
-                m_sceneGraph.removeUserCount(!isChildAgent);
-
-                // TODO: We shouldn't use closeChildAgents here - it's being used by the NPC module to stop
-                // unnecessary operations.  This should go away once NPCs have no accompanying IClientAPI
-                if (closeChildAgents && CapsModule != null)
-                    CapsModule.RemoveCaps(agentID);
-
-                // REFACTORING PROBLEM -- well not really a problem, but just to point out that whatever
-                // this method is doing is HORRIBLE!!!
-                avatar.Scene.NeedSceneCacheClear(avatar.UUID);
-
-                if (closeChildAgents && !isChildAgent)
+            lock (acd)
+            {
+                ScenePresence avatar = GetScenePresence(agentID);
+    
+                if (avatar == null)
                 {
-                    List<ulong> regions = avatar.KnownRegionHandles;
-                    regions.Remove(RegionInfo.RegionHandle);
-                    m_sceneGridService.SendCloseChildAgentConnections(agentID, regions);
+                    m_log.WarnFormat(
+                        "[SCENE]: Called RemoveClient() with agent ID {0} but no such presence is in the scene.", agentID);
+    
+                    return;
                 }
-
-                m_eventManager.TriggerClientClosed(agentID, this);
-                m_eventManager.TriggerOnRemovePresence(agentID);
-
-                if (!isChildAgent)
+    
+                try
                 {
-                    if (AttachmentsModule != null)
+                    isChildAgent = avatar.IsChildAgent;
+
+                    m_log.DebugFormat(
+                        "[SCENE]: Removing {0} agent {1} {2} from {3}",
+                        (isChildAgent ? "child" : "root"), avatar.Name, agentID, RegionInfo.RegionName);
+    
+                    // Don't do this to root agents, it's not nice for the viewer
+                    if (closeChildAgents && isChildAgent)
                     {
-                        AttachmentsModule.DeRezAttachments(avatar);
+                        // Tell a single agent to disconnect from the region.
+                        IEventQueue eq = RequestModuleInterface<IEventQueue>();
+                        if (eq != null)
+                        {
+                            eq.DisableSimulator(RegionInfo.RegionHandle, avatar.UUID);
+                        }
+                        else
+                        {
+                            avatar.ControllingClient.SendShutdownConnectionNotice();
+                        }
                     }
-
-                    ForEachClient(
-                        delegate(IClientAPI client)
+    
+                    // Only applies to root agents.
+                    if (avatar.ParentID != 0)
+                    {
+                        avatar.StandUp();
+                    }
+    
+                    m_sceneGraph.removeUserCount(!isChildAgent);
+    
+                    // TODO: We shouldn't use closeChildAgents here - it's being used by the NPC module to stop
+                    // unnecessary operations.  This should go away once NPCs have no accompanying IClientAPI
+                    if (closeChildAgents && CapsModule != null)
+                        CapsModule.RemoveCaps(agentID);
+    
+                    // REFACTORING PROBLEM -- well not really a problem, but just to point out that whatever
+                    // this method is doing is HORRIBLE!!!
+                    avatar.Scene.NeedSceneCacheClear(avatar.UUID);
+    
+                    if (closeChildAgents && !isChildAgent)
+                    {
+                        List<ulong> regions = avatar.KnownRegionHandles;
+                        regions.Remove(RegionInfo.RegionHandle);
+                        m_sceneGridService.SendCloseChildAgentConnections(agentID, regions);
+                    }
+    
+                    m_eventManager.TriggerClientClosed(agentID, this);
+                    m_eventManager.TriggerOnRemovePresence(agentID);
+    
+                    if (!isChildAgent)
+                    {
+                        if (AttachmentsModule != null)
                         {
-                            //We can safely ignore null reference exceptions.  It means the avatar is dead and cleaned up anyway
-                            try { client.SendKillObject(avatar.RegionHandle, new List<uint> { avatar.LocalId }); }
-                            catch (NullReferenceException) { }
-                        });
-                }
-
-                // It's possible for child agents to have transactions if changes are being made cross-border.
-                if (AgentTransactionsModule != null)
-                    AgentTransactionsModule.RemoveAgentAssetTransactions(agentID);
+                            AttachmentsModule.DeRezAttachments(avatar);
+                        }
 
-                m_authenticateHandler.RemoveCircuit(avatar.ControllingClient.CircuitCode);
-            }
-            catch (Exception e)
-            {
-                m_log.Error(
-                    string.Format("[SCENE]: Exception removing {0} from {1}.  Cleaning up.  Exception ", avatar.Name, Name), e);
-            }
-            finally
-            {
-                try
-                {
-                    // Always clean these structures up so that any failure above doesn't cause them to remain in the
-                    // scene with possibly bad effects (e.g. continually timing out on unacked packets and triggering
-                    // the same cleanup exception continually.
-                    m_sceneGraph.RemoveScenePresence(agentID);
-                    m_clientManager.Remove(agentID);
+                        ForEachClient(
+                            delegate(IClientAPI client)
+                            {
+                                //We can safely ignore null reference exceptions.  It means the avatar is dead and cleaned up anyway
+                                try { client.SendKillObject(avatar.RegionHandle, new List<uint> { avatar.LocalId }); }
+                                catch (NullReferenceException) { }
+                            });
+                    }
     
-                    avatar.Close();
+                    // It's possible for child agents to have transactions if changes are being made cross-border.
+                    if (AgentTransactionsModule != null)
+                        AgentTransactionsModule.RemoveAgentAssetTransactions(agentID);
                 }
                 catch (Exception e)
                 {
                     m_log.Error(
-                        string.Format("[SCENE]: Exception in final clean up of {0} in {1}.  Exception ", avatar.Name, Name), e);
+                        string.Format("[SCENE]: Exception removing {0} from {1}.  Cleaning up.  Exception ", avatar.Name, Name), e);
+                }
+                finally
+                {
+                    try
+                    {
+                        // Always clean these structures up so that any failure above doesn't cause them to remain in the
+                        // scene with possibly bad effects (e.g. continually timing out on unacked packets and triggering
+                        // the same cleanup exception continually.
+                        m_sceneGraph.RemoveScenePresence(agentID);
+                        m_clientManager.Remove(agentID);
+        
+                        avatar.Close();
+                    }
+                    catch (Exception e)
+                    {
+                        m_log.Error(
+                            string.Format("[SCENE]: Exception in final clean up of {0} in {1}.  Exception ", avatar.Name, Name), e);
+                    }
                 }
             }
 
@@ -3572,87 +3617,97 @@ namespace OpenSim.Region.Framework.Scenes
                     agent.firstname, agent.lastname, agent.Viewer);
                 reason = "Access denied, your viewer is banned by the region owner";
                 return false;
-            }           
-
-            ScenePresence sp = GetScenePresence(agent.AgentID);
-
-            if (sp != null && !sp.IsChildAgent)
-            {
-                // 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.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(true);
-                sp = null;
             }
 
-            ILandObject land = LandChannel.GetLandObject(agent.startpos.X, agent.startpos.Y);
+            ILandObject land;
 
-            //On login test land permisions
-            if (vialogin)
+            lock (agent)
             {
-                if (land != null && !TestLandRestrictions(agent, land, out reason))
+                ScenePresence sp = GetScenePresence(agent.AgentID);
+    
+                if (sp != null && !sp.IsChildAgent)
                 {
-                    return false;
+                    // 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.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(true);
+                    sp = null;
                 }
-            }
-
-            if (sp == null) // We don't have an [child] agent here already
-            {
-                if (requirePresenceLookup)
+    
+                land = LandChannel.GetLandObject(agent.startpos.X, agent.startpos.Y);
+    
+                //On login test land permisions
+                if (vialogin)
                 {
-                    try
-                    {
-                        if (!VerifyUserPresence(agent, out reason))
-                            return false;
-                    } catch (Exception e)
+                    if (land != null && !TestLandRestrictions(agent, land, out reason))
                     {
-                        m_log.ErrorFormat(
-                            "[SCENE]: Exception verifying presence {0}{1}", e.Message, e.StackTrace);
                         return false;
                     }
                 }
-
-                try
-                {
-                    if (!AuthorizeUser(agent, out reason))
-                        return false;
-                } catch (Exception e)
-                {
-                    m_log.ErrorFormat(
-                        "[SCENE]: Exception authorizing user {0}{1}", e.Message, e.StackTrace);
-                    return false;
-                }
-
-                m_log.InfoFormat(
-                    "[SCENE]: Region {0} authenticated and authorized incoming {1} agent {2} {3} {4} (circuit code {5})",
-                    RegionInfo.RegionName, (agent.child ? "child" : "root"), agent.firstname, agent.lastname,
-                    agent.AgentID, agent.circuitcode);
-
-                if (CapsModule != null)
+    
+                if (sp == null) // We don't have an [child] agent here already
                 {
-                    CapsModule.SetAgentCapsSeeds(agent);
-                    CapsModule.CreateCaps(agent.AgentID);
-                }
-            } else
-            {
-                // Let the SP know how we got here. This has a lot of interesting
-                // uses down the line.
-                sp.TeleportFlags = (TPFlags)teleportFlags;
+                    if (requirePresenceLookup)
+                    {
+                        try
+                        {
+                            if (!VerifyUserPresence(agent, out reason))
+                                return false;
+                        }
+                        catch (Exception e)
+                        {
+                            m_log.ErrorFormat(
+                                "[SCENE]: Exception verifying presence {0}{1}", e.Message, e.StackTrace);
 
-                if (sp.IsChildAgent)
-                {
-                    m_log.DebugFormat(
-                        "[SCENE]: Adjusting known seeds for existing agent {0} in {1}",
-                        agent.AgentID, RegionInfo.RegionName);
+                            return false;
+                        }
+                    }
+    
+                    try
+                    {
+                        if (!AuthorizeUser(agent, out reason))
+                            return false;
+                    }
+                    catch (Exception e)
+                    {
+                        m_log.ErrorFormat(
+                            "[SCENE]: Exception authorizing user {0}{1}", e.Message, e.StackTrace);
 
-                    sp.AdjustKnownSeeds();
-                    
+                        return false;
+                    }
+    
+                    m_log.InfoFormat(
+                        "[SCENE]: Region {0} authenticated and authorized incoming {1} agent {2} {3} {4} (circuit code {5})",
+                        RegionInfo.RegionName, (agent.child ? "child" : "root"), agent.firstname, agent.lastname,
+                        agent.AgentID, agent.circuitcode);
+    
                     if (CapsModule != null)
+                    {
                         CapsModule.SetAgentCapsSeeds(agent);
+                        CapsModule.CreateCaps(agent.AgentID);
+                    }
+                }
+                else
+                {
+                    // Let the SP know how we got here. This has a lot of interesting
+                    // uses down the line.
+                    sp.TeleportFlags = (TPFlags)teleportFlags;
+    
+                    if (sp.IsChildAgent)
+                    {
+                        m_log.DebugFormat(
+                            "[SCENE]: Adjusting known seeds for existing agent {0} in {1}",
+                            agent.AgentID, RegionInfo.RegionName);
+    
+                        sp.AdjustKnownSeeds();
+                        
+                        if (CapsModule != null)
+                            CapsModule.SetAgentCapsSeeds(agent);
+                    }
                 }
             }