From 30f4a33f01751d151ae3923b48e6e98083791140 Mon Sep 17 00:00:00 2001 From: Justin Clark-Casey (justincc) Date: Fri, 8 Jun 2012 01:24:44 +0100 Subject: 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. --- OpenSim/Region/Framework/Scenes/Scene.cs | 149 ++++++++++----------- OpenSim/Region/Framework/Scenes/ScenePresence.cs | 2 +- .../Scenes/Tests/ScenePresenceAgentTests.cs | 4 +- 3 files changed, 74 insertions(+), 81 deletions(-) (limited to 'OpenSim/Region/Framework/Scenes') 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 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 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(); - // 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) { } - }); + 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); } - // 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 { 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 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] -- cgit v1.1