From 73e9b0be725a73a489b29f3fe2df236c897ef3b5 Mon Sep 17 00:00:00 2001 From: Dan Lake Date: Wed, 17 Mar 2010 06:40:00 -0700 Subject: Inconsistent locking of ScenePresence array in SceneGraph. Fixed by eliminating option to return the actual list. Callers can now either request a copy of the array as a new List or ask the SceneGraph to call a delegate function on every ScenePresence. Iteration and locking of the ScenePresences now takes place only within the SceneGraph class. This patch also applies a fix to Combat/CombatModule.cs which had unlocked iteration of the ScenePresences and inconsistent try/catch around the use of those ScenePresences. --- OpenSim/Region/Framework/Scenes/Scene.cs | 52 +++--- OpenSim/Region/Framework/Scenes/SceneGraph.cs | 193 ++++++++++++++------- OpenSim/Region/Framework/Scenes/SceneManager.cs | 17 +- .../Region/Framework/Scenes/SceneObjectGroup.cs | 12 +- OpenSim/Region/Framework/Scenes/SceneObjectPart.cs | 82 ++++----- OpenSim/Region/Framework/Scenes/ScenePresence.cs | 20 +-- 6 files changed, 211 insertions(+), 165 deletions(-) (limited to 'OpenSim/Region/Framework/Scenes') diff --git a/OpenSim/Region/Framework/Scenes/Scene.cs b/OpenSim/Region/Framework/Scenes/Scene.cs index baa8759..a86a33c 100644 --- a/OpenSim/Region/Framework/Scenes/Scene.cs +++ b/OpenSim/Region/Framework/Scenes/Scene.cs @@ -3599,9 +3599,7 @@ namespace OpenSim.Region.Framework.Scenes public virtual void AgentCrossing(UUID agentID, Vector3 position, bool isFlying) { ScenePresence presence; - m_sceneGraph.TryGetAvatar(agentID, out presence); - - if (presence != null) + if(m_sceneGraph.TryGetAvatar(agentID, out presence)) { try { @@ -3776,9 +3774,7 @@ namespace OpenSim.Region.Framework.Scenes Vector3 lookAt, uint teleportFlags) { ScenePresence sp; - m_sceneGraph.TryGetAvatar(remoteClient.AgentId, out sp); - - if (sp != null) + if(m_sceneGraph.TryGetAvatar(remoteClient.AgentId, out sp)) { uint regionX = m_regInfo.RegionLocX; uint regionY = m_regInfo.RegionLocY; @@ -4134,6 +4130,11 @@ namespace OpenSim.Region.Framework.Scenes //The idea is to have a group of method that return a list of avatars meeting some requirement // ie it could be all m_scenePresences within a certain range of the calling prim/avatar. + // + // GetAvatars returns a new list of all root agent presences in the scene + // GetScenePresences returns a new list of all presences in the scene or a filter may be passed. + // GetScenePresence returns the presence with matching UUID or first/last name. + // ForEachScenePresence requests the Scene to run a delegate function against all presences. /// /// Return a list of all avatars in this region. @@ -4150,7 +4151,7 @@ namespace OpenSim.Region.Framework.Scenes /// This list is a new object, so it can be iterated over without locking. /// /// - public ScenePresence[] GetScenePresences() + public List GetScenePresences() { return m_sceneGraph.GetScenePresences(); } @@ -4176,6 +4177,28 @@ namespace OpenSim.Region.Framework.Scenes return m_sceneGraph.GetScenePresence(avatarID); } + /// + /// Request the ScenePresence in this region by first/last name. + /// Should normally only be a single match, but first is always returned + /// + /// + /// + /// + public ScenePresence GetScenePresence(string firstName, string lastName) + { + return m_sceneGraph.GetScenePresence(firstName, lastName); + } + + /// + /// Request the ScenePresence in this region by localID. + /// + /// + /// + public ScenePresence GetScenePresence(uint localID) + { + return m_sceneGraph.GetScenePresence(localID); + } + public override bool PresenceChildStatus(UUID avatarID) { ScenePresence cp = GetScenePresence(avatarID); @@ -4191,25 +4214,14 @@ namespace OpenSim.Region.Framework.Scenes } /// - /// + /// Performs action on all scene presences. /// /// public void ForEachScenePresence(Action action) { - // We don't want to try to send messages if there are no avatars. if (m_sceneGraph != null) { - try - { - ScenePresence[] presences = GetScenePresences(); - for (int i = 0; i < presences.Length; i++) - action(presences[i]); - } - catch (Exception e) - { - m_log.Info("[BUG] in " + RegionInfo.RegionName + ": " + e.ToString()); - m_log.Info("[BUG] Stack Trace: " + e.StackTrace); - } + m_sceneGraph.ForEachScenePresence(action); } } diff --git a/OpenSim/Region/Framework/Scenes/SceneGraph.cs b/OpenSim/Region/Framework/Scenes/SceneGraph.cs index d944834..d259c42 100644 --- a/OpenSim/Region/Framework/Scenes/SceneGraph.cs +++ b/OpenSim/Region/Framework/Scenes/SceneGraph.cs @@ -165,9 +165,10 @@ namespace OpenSim.Region.Framework.Scenes protected internal void UpdatePresences() { - ScenePresence[] updateScenePresences = GetScenePresences(); - for (int i = 0; i < updateScenePresences.Length; i++) - updateScenePresences[i].Update(); + ForEachScenePresence(delegate(ScenePresence presence) + { + presence.Update(); + }); } protected internal float UpdatePhysics(double elapsed) @@ -196,9 +197,10 @@ namespace OpenSim.Region.Framework.Scenes protected internal void UpdateScenePresenceMovement() { - ScenePresence[] moveEntities = GetScenePresences(); - for (int i = 0; i < moveEntities.Length; i++) - moveEntities[i].UpdateMovement(); + ForEachScenePresence(delegate(ScenePresence presence) + { + presence.UpdateMovement(); + }); } #endregion @@ -616,18 +618,16 @@ namespace OpenSim.Region.Framework.Scenes public void RecalculateStats() { - ScenePresence[] presences = GetScenePresences(); int rootcount = 0; int childcount = 0; - for (int i = 0; i < presences.Length; i++) + ForEachScenePresence(delegate(ScenePresence presence) { - ScenePresence user = presences[i]; - if (user.IsChildAgent) + if (presence.IsChildAgent) ++childcount; else ++rootcount; - } + }); m_numRootAgents = rootcount; m_numChildAgents = childcount; @@ -674,25 +674,6 @@ namespace OpenSim.Region.Framework.Scenes #endregion #region Get Methods - - /// - /// Request a List of all scene presences in this scene. This is a new list, so no - /// locking is required to iterate over it. - /// - /// - protected internal ScenePresence[] GetScenePresences() - { - return m_scenePresenceArray; - } - - protected internal List GetAvatars() - { - List result = - GetScenePresences(delegate(ScenePresence scenePresence) { return !scenePresence.IsChildAgent; }); - - return result; - } - /// /// Get the controlling client for the given avatar, if there is one. /// @@ -718,45 +699,119 @@ namespace OpenSim.Region.Framework.Scenes return null; } + // The idea is to have a group of method that return a list of avatars meeting some requirement + // ie it could be all m_scenePresences within a certain range of the calling prim/avatar. + // + // GetAvatars returns a new list of all root agent presences in the scene + // GetScenePresences returns a new list of all presences in the scene or a filter may be passed. + // GetScenePresence returns the presence with matching UUID or the first presence matching the passed filter. + // ForEachScenePresence requests the Scene to run a delegate function against all presences. + + /// + /// Request a list of all avatars in this region (no child agents) + /// This list is a new object, so it can be iterated over without locking. + /// + /// + public List GetAvatars() + { + return GetScenePresences(delegate(ScenePresence scenePresence) + { + return !scenePresence.IsChildAgent; + }); + } + + /// + /// Request a list of m_scenePresences in this World + /// Returns a copy so it can be iterated without a lock. + /// There is no guarantee that presences will remain in the scene after the list is returned. + /// + /// + protected internal List GetScenePresences() + { + List result; + lock (m_scenePresences) + { + result = new List(m_scenePresenceArray.Length); + result.AddRange(m_scenePresenceArray); + } + return result; + } + /// /// Request a filtered list of m_scenePresences in this World + /// Returns a copy so it can be iterated without a lock. + /// There is no guarantee that presences will remain in the scene after the list is returned. /// /// protected internal List GetScenePresences(FilterAvatarList filter) { - // No locking of scene presences here since we're passing back a list... - List result = new List(); - ScenePresence[] scenePresences = GetScenePresences(); + // Check each ScenePresence against the filter + ForEachScenePresence(delegate(ScenePresence presence) + { + if (filter(presence)) + result.Add(presence); + }); + return result; + } - for (int i = 0; i < scenePresences.Length; i++) + /// + /// Request the ScenePresence in this region matching filter. + /// Only the first match is returned. + /// + /// + /// + /// + protected internal ScenePresence GetScenePresence(FilterAvatarList filter) + { + ScenePresence result = null; + // Get all of the ScenePresences + List presences = GetScenePresences(); + foreach (ScenePresence presence in presences) { - ScenePresence avatar = scenePresences[i]; - if (filter(avatar)) - result.Add(avatar); + if (filter(presence)) + { + result = presence; + break; + } } - return result; } + protected internal ScenePresence GetScenePresence(string firstName, string lastName) + { + return GetScenePresence(delegate(ScenePresence presence) + { + return(presence.Firstname == firstName && presence.Lastname == lastName); + }); + } + /// /// Request a scene presence by UUID /// - /// + /// /// null if the agent was not found protected internal ScenePresence GetScenePresence(UUID agentID) { ScenePresence sp; - - lock (m_scenePresences) - { - m_scenePresences.TryGetValue(agentID, out sp); - } - + TryGetAvatar(agentID, out sp); return sp; } /// + /// Request the ScenePresence in this region by localID. + /// + /// + /// + protected internal ScenePresence GetScenePresence(uint localID) + { + return GetScenePresence(delegate(ScenePresence presence) + { + return (presence.LocalId == localID); + }); + } + + /// /// Get a scene object group that contains the prim with the given local id /// /// @@ -910,29 +965,19 @@ namespace OpenSim.Region.Framework.Scenes protected internal bool TryGetAvatar(UUID avatarId, out ScenePresence avatar) { lock (m_scenePresences) - return m_scenePresences.TryGetValue(avatarId, out avatar); + { + m_scenePresences.TryGetValue(avatarId, out avatar); + } + return (avatar != null); } protected internal bool TryGetAvatarByName(string avatarName, out ScenePresence avatar) { - ScenePresence[] presences = GetScenePresences(); - - for (int i = 0; i < presences.Length; i++) + avatar = GetScenePresence(delegate(ScenePresence presence) { - ScenePresence presence = presences[i]; - - if (!presence.IsChildAgent) - { - if (String.Compare(avatarName, presence.ControllingClient.Name, true) == 0) - { - avatar = presence; - return true; - } - } - } - - avatar = null; - return false; + return (String.Compare(avatarName, presence.ControllingClient.Name, true) == 0); + }); + return (avatar != null); } /// @@ -1013,6 +1058,28 @@ namespace OpenSim.Region.Framework.Scenes } } } + + + /// + /// Performs action on all scene presences. + /// + /// + public void ForEachScenePresence(Action action) + { + List presences = GetScenePresences(); + try + { + foreach(ScenePresence presence in presences) + { + action(presence); + } + } + catch (Exception e) + { + m_log.Info("[BUG] in " + m_parentScene.RegionInfo.RegionName + ": " + e.ToString()); + m_log.Info("[BUG] Stack Trace: " + e.StackTrace); + } + } #endregion diff --git a/OpenSim/Region/Framework/Scenes/SceneManager.cs b/OpenSim/Region/Framework/Scenes/SceneManager.cs index 680c39a..a955532 100644 --- a/OpenSim/Region/Framework/Scenes/SceneManager.cs +++ b/OpenSim/Region/Framework/Scenes/SceneManager.cs @@ -414,12 +414,8 @@ namespace OpenSim.Region.Framework.Scenes ForEachCurrentScene( delegate(Scene scene) { - ScenePresence[] scenePresences = scene.GetScenePresences(); - - for (int i = 0; i < scenePresences.Length; i++) + scene.ForEachScenePresence(delegate(ScenePresence scenePresence) { - ScenePresence scenePresence = scenePresences[i]; - if (!scenePresence.IsChildAgent) { m_log.DebugFormat("Packet debug for {0} {1} set to {2}", @@ -429,7 +425,7 @@ namespace OpenSim.Region.Framework.Scenes scenePresence.ControllingClient.SetDebugPacketLevel(newDebug); } - } + }); } ); } @@ -441,14 +437,11 @@ namespace OpenSim.Region.Framework.Scenes ForEachCurrentScene( delegate(Scene scene) { - ScenePresence[] scenePresences = scene.GetScenePresences(); - - for (int i = 0; i < scenePresences.Length; i++) + scene.ForEachScenePresence(delegate(ScenePresence scenePresence) { - ScenePresence scenePresence = scenePresences[i]; if (!scenePresence.IsChildAgent) avatars.Add(scenePresence); - } + }); } ); @@ -461,7 +454,7 @@ namespace OpenSim.Region.Framework.Scenes ForEachCurrentScene(delegate(Scene scene) { - ScenePresence[] scenePresences = scene.GetScenePresences(); + List scenePresences = scene.GetScenePresences(); presences.AddRange(scenePresences); }); diff --git a/OpenSim/Region/Framework/Scenes/SceneObjectGroup.cs b/OpenSim/Region/Framework/Scenes/SceneObjectGroup.cs index 88deedf..8aefd50 100644 --- a/OpenSim/Region/Framework/Scenes/SceneObjectGroup.cs +++ b/OpenSim/Region/Framework/Scenes/SceneObjectGroup.cs @@ -1268,22 +1268,20 @@ namespace OpenSim.Region.Framework.Scenes foreach (SceneObjectPart part in m_parts.Values) { // part.Inventory.RemoveScriptInstances(); - - ScenePresence[] avatars = Scene.GetScenePresences(); - for (int i = 0; i < avatars.Length; i++) + Scene.ForEachScenePresence(delegate(ScenePresence avatar) { - if (avatars[i].ParentID == LocalId) + if (avatar.ParentID == LocalId) { - avatars[i].StandUp(); + avatar.StandUp(); } if (!silent) { part.UpdateFlag = 0; if (part == m_rootPart) - avatars[i].ControllingClient.SendKillObject(m_regionHandle, part.LocalId); + avatar.ControllingClient.SendKillObject(m_regionHandle, part.LocalId); } - } + }); } } } diff --git a/OpenSim/Region/Framework/Scenes/SceneObjectPart.cs b/OpenSim/Region/Framework/Scenes/SceneObjectPart.cs index a2b98b9..0e21487 100644 --- a/OpenSim/Region/Framework/Scenes/SceneObjectPart.cs +++ b/OpenSim/Region/Framework/Scenes/SceneObjectPart.cs @@ -1197,15 +1197,14 @@ namespace OpenSim.Region.Framework.Scenes private void SendObjectPropertiesToClient(UUID AgentID) { - ScenePresence[] avatars = m_parentGroup.Scene.GetScenePresences(); - for (int i = 0; i < avatars.Length; i++) + m_parentGroup.Scene.ForEachScenePresence(delegate(ScenePresence avatar) { // Ugly reference :( - if (avatars[i].UUID == AgentID) + if (avatar.UUID == AgentID) { - m_parentGroup.GetProperties(avatars[i].ControllingClient); + m_parentGroup.GetProperties(avatar.ControllingClient); } - } + }); } // TODO: unused: @@ -1260,11 +1259,10 @@ namespace OpenSim.Region.Framework.Scenes /// public void AddFullUpdateToAllAvatars() { - ScenePresence[] avatars = m_parentGroup.Scene.GetScenePresences(); - for (int i = 0; i < avatars.Length; i++) + m_parentGroup.Scene.ForEachScenePresence(delegate(ScenePresence avatar) { - avatars[i].SceneViewer.QueuePartForUpdate(this); - } + avatar.SceneViewer.QueuePartForUpdate(this); + }); } public void AddFullUpdateToAvatar(ScenePresence presence) @@ -1285,11 +1283,10 @@ namespace OpenSim.Region.Framework.Scenes /// Terse updates public void AddTerseUpdateToAllAvatars() { - ScenePresence[] avatars = m_parentGroup.Scene.GetScenePresences(); - for (int i = 0; i < avatars.Length; i++) + m_parentGroup.Scene.ForEachScenePresence(delegate(ScenePresence avatar) { - avatars[i].SceneViewer.QueuePartForUpdate(this); - } + avatar.SceneViewer.QueuePartForUpdate(this); + }); } public void AddTerseUpdateToAvatar(ScenePresence presence) @@ -2121,17 +2118,13 @@ namespace OpenSim.Region.Framework.Scenes } else { - ScenePresence[] avlist = m_parentGroup.Scene.GetScenePresences(); - - for (int i = 0; i < avlist.Length; i++) + m_parentGroup.Scene.ForEachScenePresence(delegate(ScenePresence av) { - ScenePresence av = avlist[i]; - if (av.LocalId == localId) { if (m_parentGroup.RootPart.CollisionFilter.ContainsValue(av.UUID.ToString()) || m_parentGroup.RootPart.CollisionFilter.ContainsValue(av.Name)) { - bool found = m_parentGroup.RootPart.CollisionFilter.TryGetValue(1,out data); + bool found = m_parentGroup.RootPart.CollisionFilter.TryGetValue(1, out data); //If it is 1, it is to accept ONLY collisions from this avatar if (found) { @@ -2153,7 +2146,7 @@ namespace OpenSim.Region.Framework.Scenes } else { - bool found = m_parentGroup.RootPart.CollisionFilter.TryGetValue(1,out data); + bool found = m_parentGroup.RootPart.CollisionFilter.TryGetValue(1, out data); //If it is 1, it is to accept ONLY collisions from this avatar, so this other avatar will not work if (!found) { @@ -2171,7 +2164,7 @@ namespace OpenSim.Region.Framework.Scenes } } - } + }); } } if (colliding.Count > 0) @@ -2257,17 +2250,13 @@ namespace OpenSim.Region.Framework.Scenes } else { - ScenePresence[] avlist = m_parentGroup.Scene.GetScenePresences(); - - for (int i = 0; i < avlist.Length; i++) + m_parentGroup.Scene.ForEachScenePresence(delegate(ScenePresence av) { - ScenePresence av = avlist[i]; - if (av.LocalId == localId) { if (m_parentGroup.RootPart.CollisionFilter.ContainsValue(av.UUID.ToString()) || m_parentGroup.RootPart.CollisionFilter.ContainsValue(av.Name)) { - bool found = m_parentGroup.RootPart.CollisionFilter.TryGetValue(1,out data); + bool found = m_parentGroup.RootPart.CollisionFilter.TryGetValue(1, out data); //If it is 1, it is to accept ONLY collisions from this avatar if (found) { @@ -2289,7 +2278,7 @@ namespace OpenSim.Region.Framework.Scenes } else { - bool found = m_parentGroup.RootPart.CollisionFilter.TryGetValue(1,out data); + bool found = m_parentGroup.RootPart.CollisionFilter.TryGetValue(1, out data); //If it is 1, it is to accept ONLY collisions from this avatar, so this other avatar will not work if (!found) { @@ -2307,7 +2296,7 @@ namespace OpenSim.Region.Framework.Scenes } } - } + }); } } if (colliding.Count > 0) @@ -2388,17 +2377,13 @@ namespace OpenSim.Region.Framework.Scenes } else { - ScenePresence[] avlist = m_parentGroup.Scene.GetScenePresences(); - - for (int i = 0; i < avlist.Length; i++) + m_parentGroup.Scene.ForEachScenePresence(delegate(ScenePresence av) { - ScenePresence av = avlist[i]; - if (av.LocalId == localId) { if (m_parentGroup.RootPart.CollisionFilter.ContainsValue(av.UUID.ToString()) || m_parentGroup.RootPart.CollisionFilter.ContainsValue(av.Name)) { - bool found = m_parentGroup.RootPart.CollisionFilter.TryGetValue(1,out data); + bool found = m_parentGroup.RootPart.CollisionFilter.TryGetValue(1, out data); //If it is 1, it is to accept ONLY collisions from this avatar if (found) { @@ -2420,7 +2405,7 @@ namespace OpenSim.Region.Framework.Scenes } else { - bool found = m_parentGroup.RootPart.CollisionFilter.TryGetValue(1,out data); + bool found = m_parentGroup.RootPart.CollisionFilter.TryGetValue(1, out data); //If it is 1, it is to accept ONLY collisions from this avatar, so this other avatar will not work if (!found) { @@ -2438,7 +2423,7 @@ namespace OpenSim.Region.Framework.Scenes } } - } + }); } } @@ -2877,11 +2862,10 @@ namespace OpenSim.Region.Framework.Scenes /// public void SendFullUpdateToAllClients() { - ScenePresence[] avatars = m_parentGroup.Scene.GetScenePresences(); - for (int i = 0; i < avatars.Length; i++) + m_parentGroup.Scene.ForEachScenePresence(delegate(ScenePresence avatar) { - SendFullUpdate(avatars[i].ControllingClient, avatars[i].GenerateClientFlags(UUID)); - } + SendFullUpdate(avatar.ControllingClient, avatar.GenerateClientFlags(UUID)); + }); } /// @@ -2890,13 +2874,12 @@ namespace OpenSim.Region.Framework.Scenes /// public void SendFullUpdateToAllClientsExcept(UUID agentID) { - ScenePresence[] avatars = m_parentGroup.Scene.GetScenePresences(); - for (int i = 0; i < avatars.Length; i++) + m_parentGroup.Scene.ForEachScenePresence(delegate(ScenePresence avatar) { // Ugly reference :( - if (avatars[i].UUID != agentID) - SendFullUpdate(avatars[i].ControllingClient, avatars[i].GenerateClientFlags(UUID)); - } + if (avatar.UUID != agentID) + SendFullUpdate(avatar.ControllingClient, avatar.GenerateClientFlags(UUID)); + }); } /// @@ -3096,11 +3079,10 @@ namespace OpenSim.Region.Framework.Scenes /// public void SendTerseUpdateToAllClients() { - ScenePresence[] avatars = m_parentGroup.Scene.GetScenePresences(); - for (int i = 0; i < avatars.Length; i++) + m_parentGroup.Scene.ForEachScenePresence(delegate(ScenePresence avatar) { - SendTerseUpdateToClient(avatars[i].ControllingClient); - } + SendTerseUpdateToClient(avatar.ControllingClient); + }); } public void SetAttachmentPoint(uint AttachmentPoint) diff --git a/OpenSim/Region/Framework/Scenes/ScenePresence.cs b/OpenSim/Region/Framework/Scenes/ScenePresence.cs index 6f16ff3..766f6d3 100644 --- a/OpenSim/Region/Framework/Scenes/ScenePresence.cs +++ b/OpenSim/Region/Framework/Scenes/ScenePresence.cs @@ -912,14 +912,11 @@ namespace OpenSim.Region.Framework.Scenes m_isChildAgent = false; - ScenePresence[] animAgents = m_scene.GetScenePresences(); - for (int i = 0; i < animAgents.Length; i++) + m_scene.ForEachScenePresence(delegate(ScenePresence presence) { - ScenePresence presence = animAgents[i]; - if (presence != this) presence.Animator.SendAnimPackToClient(ControllingClient); - } + }); m_scene.EventManager.TriggerOnMakeRootAgent(this); } @@ -2469,13 +2466,10 @@ namespace OpenSim.Region.Framework.Scenes public void SendInitialFullUpdateToAllClients() { m_perfMonMS = Util.EnvironmentTickCount(); - - ScenePresence[] avatars = m_scene.GetScenePresences(); - - for (int i = 0; i < avatars.Length; i++) + int avUpdates = 0; + m_scene.ForEachScenePresence(delegate(ScenePresence avatar) { - ScenePresence avatar = avatars[i]; - + ++avUpdates; // only send if this is the root (children are only "listening posts" in a foreign region) if (!IsChildAgent) { @@ -2491,9 +2485,9 @@ namespace OpenSim.Region.Framework.Scenes avatar.Animator.SendAnimPackToClient(ControllingClient); } } - } + }); - m_scene.StatsReporter.AddAgentUpdates(avatars.Length); + m_scene.StatsReporter.AddAgentUpdates(avUpdates); m_scene.StatsReporter.AddAgentTime(Util.EnvironmentTickCountSubtract(m_perfMonMS)); //Animator.SendAnimPack(); -- cgit v1.1