From 498154af808062a717359e06010c8365c6e7cbf1 Mon Sep 17 00:00:00 2001 From: "Justin Clark-Casey (justincc)" Date: Fri, 8 Jun 2012 01:24:44 +0100 Subject: [PATCH] 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. --- .../EntityTransfer/EntityTransferModule.cs | 1 - OpenSim/Region/Framework/Scenes/Scene.cs | 155 +++++++++--------- .../Region/Framework/Scenes/ScenePresence.cs | 2 +- .../Scenes/Tests/ScenePresenceAgentTests.cs | 4 +- 4 files changed, 77 insertions(+), 85 deletions(-) diff --git a/OpenSim/Region/CoreModules/Framework/EntityTransfer/EntityTransferModule.cs b/OpenSim/Region/CoreModules/Framework/EntityTransfer/EntityTransferModule.cs index 815a3252ca..14d8e69d49 100644 --- a/OpenSim/Region/CoreModules/Framework/EntityTransfer/EntityTransferModule.cs +++ b/OpenSim/Region/CoreModules/Framework/EntityTransfer/EntityTransferModule.cs @@ -638,7 +638,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 7232d26424..e7f79b537f 100644 --- a/OpenSim/Region/Framework/Scenes/Scene.cs +++ b/OpenSim/Region/Framework/Scenes/Scene.cs @@ -3211,10 +3211,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) { @@ -3236,99 +3249,77 @@ namespace OpenSim.Region.Framework.Scenes avatar.StandUp(); } - try + 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) { - m_log.DebugFormat( - "[SCENE]: Removing {0} agent {1} {2} from region {3}", - (isChildAgent ? "child" : "root"), avatar.Name, agentID, RegionInfo.RegionName); + List regions = avatar.KnownRegionHandles; + regions.Remove(RegionInfo.RegionHandle); + m_sceneGridService.SendCloseChildAgentConnections(agentID, regions); + } - m_sceneGraph.removeUserCount(!isChildAgent); + m_eventManager.TriggerClientClosed(agentID, this); + m_eventManager.TriggerOnRemovePresence(agentID); - // 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 && !avatar.IsChildAgent) + if (!isChildAgent) + { + if (AttachmentsModule != null && avatar.PresenceType != PresenceType.Npc) { - List regions = avatar.KnownRegionHandles; - regions.Remove(RegionInfo.RegionHandle); - m_sceneGridService.SendCloseChildAgentConnections(agentID, regions); + IUserManagement uMan = RequestModuleInterface(); + // 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); } - m_eventManager.TriggerClientClosed(agentID, this); - } - catch (NullReferenceException) - { - // We don't know which count to remove it from - // Avatar is already disposed :/ - } - - try - { - m_eventManager.TriggerOnRemovePresence(agentID); - - if (!isChildAgent) - { - if (AttachmentsModule != null && avatar.PresenceType != PresenceType.Npc) + ForEachClient( + delegate(IClientAPI client) { - IUserManagement uMan = RequestModuleInterface(); - // 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 { 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); - } - 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); + //We can safely ignore null reference exceptions. It means the avatar is dead and cleaned up anyway + try { client.SendKillObject(avatar.RegionHandle, new List { 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)); } /// diff --git a/OpenSim/Region/Framework/Scenes/ScenePresence.cs b/OpenSim/Region/Framework/Scenes/ScenePresence.cs index 9b157501dd..7a7e90e576 100644 --- a/OpenSim/Region/Framework/Scenes/ScenePresence.cs +++ b/OpenSim/Region/Framework/Scenes/ScenePresence.cs @@ -3437,7 +3437,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 1aa48d7520..02c45ef11c 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]