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/SceneGraph.cs | 193 +++++++++++++++++--------- 1 file changed, 130 insertions(+), 63 deletions(-) (limited to 'OpenSim/Region/Framework/Scenes/SceneGraph.cs') 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 -- cgit v1.1