Don't make duplicate call to ScenePresence.Close() separately in ETM.DoTeleport() if an agent needs closing.

This is always done as part of Scene.RemoveClient()
Also refactors try/catching in Scene.RemoveClient() to log NREs instead of silently discarding, since these are useful symptoms of problems.

4 files changed, 74 insertions, 82 deletions

diff --git a/OpenSim/Region/CoreModules/Framework/EntityTransfer/EntityTransferModule.cs b/OpenSim/Region/CoreModules/Framework/EntityTransfer/EntityTransferModule.cs
index f2926ea..7d82060 100644
--- a/OpenSim/Region/CoreModules/Framework/EntityTransfer/EntityTransferModule.cs
+++ b/OpenSim/Region/CoreModules/Framework/EntityTransfer/EntityTransferModule.cs
@@ -644,7 +644,6 @@ namespace OpenSim.Region.CoreModules.Framework.EntityTransfer
                 // an agent cannot teleport back to this region if it has teleported away.
                 Thread.Sleep(2000);
 
-                sp.Close();
                 sp.Scene.IncomingCloseAgent(sp.UUID);
             }
             else
diff --git a/OpenSim/Region/Framework/Scenes/Scene.cs b/OpenSim/Region/Framework/Scenes/Scene.cs
index 9048f00..7afde38 100644
--- a/OpenSim/Region/Framework/Scenes/Scene.cs
+++ b/OpenSim/Region/Framework/Scenes/Scene.cs
@@ -3231,10 +3231,23 @@ namespace OpenSim.Region.Framework.Scenes
 //            CheckHeartbeat();
             bool isChildAgent = false;
             ScenePresence avatar = GetScenePresence(agentID);
-            if (avatar != null)
+
+            if (avatar == null)
+            {
+                m_log.WarnFormat(
+                    "[SCENE]: Called RemoveClient() with agent ID {0} but no such presence is in the scene.", agentID);
+
+                return;
+            }
+
+            try
             {
                 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)
                 {
@@ -3256,99 +3269,77 @@ namespace OpenSim.Region.Framework.Scenes
                     avatar.StandUp();
                 }
 
-                try
-                {
-                    m_log.DebugFormat(
-                        "[SCENE]: Removing {0} agent {1} {2} from region {3}",
-                        (isChildAgent ? "child" : "root"), avatar.Name, agentID, RegionInfo.RegionName);
-
-                    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);
+                m_sceneGraph.removeUserCount(!isChildAgent);
 
-                    // 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);
+                // 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);
 
-                    if (closeChildAgents && !avatar.IsChildAgent)
-                    {
-                        List<ulong> regions = avatar.KnownRegionHandles;
-                        regions.Remove(RegionInfo.RegionHandle);
-                        m_sceneGridService.SendCloseChildAgentConnections(agentID, regions);
-                    }
+                // 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);
 
-                    m_eventManager.TriggerClientClosed(agentID, this);
-                }
-                catch (NullReferenceException)
+                if (closeChildAgents && !isChildAgent)
                 {
-                    // We don't know which count to remove it from
-                    // Avatar is already disposed :/
+                    List<ulong> regions = avatar.KnownRegionHandles;
+                    regions.Remove(RegionInfo.RegionHandle);
+                    m_sceneGridService.SendCloseChildAgentConnections(agentID, regions);
                 }
 
-                try
+                m_eventManager.TriggerClientClosed(agentID, this);
+                m_eventManager.TriggerOnRemovePresence(agentID);
+
+                if (!isChildAgent)
                 {
-                    m_eventManager.TriggerOnRemovePresence(agentID);
-    
-                    if (!isChildAgent)
+                    if (AttachmentsModule != null && avatar.PresenceType != PresenceType.Npc)
                     {
-                        if (AttachmentsModule != null && avatar.PresenceType != PresenceType.Npc)
-                        {
-                            IUserManagement uMan = RequestModuleInterface<IUserManagement>();
-                            // Don't save attachments for HG visitors, it
-                            // messes up their inventory. When a HG visitor logs
-                            // out on a foreign grid, their attachments will be
-                            // reloaded in the state they were in when they left
-                            // the home grid. This is best anyway as the visited
-                            // grid may use an incompatible script engine.
-                            if (uMan == null || uMan.IsLocalGridUser(avatar.UUID))
-                                AttachmentsModule.SaveChangedAttachments(avatar, false);
-                        }
-
-                        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) { }
-                            });
+                        IUserManagement uMan = RequestModuleInterface<IUserManagement>();
+                        // Don't save attachments for HG visitors, it
+                        // messes up their inventory. When a HG visitor logs
+                        // out on a foreign grid, their attachments will be
+                        // reloaded in the state they were in when they left
+                        // the home grid. This is best anyway as the visited
+                        // grid may use an incompatible script engine.
+                        if (uMan == null || uMan.IsLocalGridUser(avatar.UUID))
+                            AttachmentsModule.SaveChangedAttachments(avatar, false);
                     }
 
-                    // It's possible for child agents to have transactions if changes are being made cross-border.
-                    if (AgentTransactionsModule != null)
-                        AgentTransactionsModule.RemoveAgentAssetTransactions(agentID);
-                }
-                finally
-                {
-                    // 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.
-                    // TODO: This should probably extend to the whole method, but we don't want to also catch the NRE
-                    // since this would hide the underlying failure and other associated problems.
-                    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) { }
+                        });
                 }
 
-                try
-                {
-                    avatar.Close();
-                }
-                catch (NullReferenceException)
-                {
-                    //We can safely ignore null reference exceptions.  It means the avatar are dead and cleaned up anyway.
-                }
-                catch (Exception e)
-                {
-                    m_log.ErrorFormat("[SCENE] Scene.cs:RemoveClient exception {0}{1}", e.Message, e.StackTrace);
-                }
+                // It's possible for child agents to have transactions if changes are being made cross-border.
+                if (AgentTransactionsModule != null)
+                    AgentTransactionsModule.RemoveAgentAssetTransactions(agentID);
+
+                avatar.Close();
 
                 m_authenticateHandler.RemoveCircuit(avatar.ControllingClient.CircuitCode);
-//                CleanDroppedAttachments();
-                //m_log.InfoFormat("[SCENE] Memory pre  GC {0}", System.GC.GetTotalMemory(false));
-                //m_log.InfoFormat("[SCENE] Memory post GC {0}", System.GC.GetTotalMemory(true));
             }
+            catch (Exception e)
+            {
+                m_log.Error(
+                    string.Format("[SCENE]: Exception removing {0} from {1}, ", avatar.Name, RegionInfo.RegionName), e);
+            }
+            finally
+            {
+                // 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.
+                // TODO: This should probably extend to the whole method, but we don't want to also catch the NRE
+                // since this would hide the underlying failure and other associated problems.
+                m_sceneGraph.RemoveScenePresence(agentID);
+                m_clientManager.Remove(agentID);
+            }
+
+            //m_log.InfoFormat("[SCENE] Memory pre  GC {0}", System.GC.GetTotalMemory(false));
+            //m_log.InfoFormat("[SCENE] Memory post GC {0}", System.GC.GetTotalMemory(true));
         }
 
         /// <summary>
diff --git a/OpenSim/Region/Framework/Scenes/ScenePresence.cs b/OpenSim/Region/Framework/Scenes/ScenePresence.cs
index 3adafc1..029e0d7 100644
--- a/OpenSim/Region/Framework/Scenes/ScenePresence.cs
+++ b/OpenSim/Region/Framework/Scenes/ScenePresence.cs
@@ -3416,7 +3416,7 @@ namespace OpenSim.Region.Framework.Scenes
 
         public void Close()
         {
-            if (!IsChildAgent)
+            if (!IsChildAgent && m_scene.AttachmentsModule != null)
                 m_scene.AttachmentsModule.DeleteAttachmentsFromScene(this, false);
 
             // Clear known regions
diff --git a/OpenSim/Region/Framework/Scenes/Tests/ScenePresenceAgentTests.cs b/OpenSim/Region/Framework/Scenes/Tests/ScenePresenceAgentTests.cs
index 1aa48d7..02c45ef 100644
--- a/OpenSim/Region/Framework/Scenes/Tests/ScenePresenceAgentTests.cs
+++ b/OpenSim/Region/Framework/Scenes/Tests/ScenePresenceAgentTests.cs
@@ -101,7 +101,7 @@ namespace OpenSim.Region.Framework.Scenes.Tests
         public void TestCloseAgent()
         {
             TestHelpers.InMethod();
-//            log4net.Config.XmlConfigurator.Configure();
+//            TestHelpers.EnableLogging();
 
             TestScene scene = new SceneHelpers().SetupScene();
             ScenePresence sp = SceneHelpers.AddScenePresence(scene, TestHelpers.ParseTail(0x1));
@@ -114,6 +114,8 @@ namespace OpenSim.Region.Framework.Scenes.Tests
             Assert.That(scene.GetScenePresence(sp.UUID), Is.Null);
             Assert.That(scene.AuthenticateHandler.GetAgentCircuitData(sp.UUID), Is.Null);
             Assert.That(scene.AuthenticateHandler.GetAgentCircuits().Count, Is.EqualTo(0));
+
+//            TestHelpers.DisableLogging();
         }
 
         [Test]