Improve consistency of locking for SOG.m_parts in order to avoid race conditions in linking and unlinking

1 files changed, 79 insertions, 52 deletions

diff --git a/OpenSim/Region/Framework/Scenes/SceneObjectGroup.cs b/OpenSim/Region/Framework/Scenes/SceneObjectGroup.cs
index 952d280..5ee8d73 100644
--- a/OpenSim/Region/Framework/Scenes/SceneObjectGroup.cs
+++ b/OpenSim/Region/Framework/Scenes/SceneObjectGroup.cs
@@ -213,7 +213,7 @@ namespace OpenSim.Region.Framework.Scenes
         /// </summary>
         public int PrimCount
         {
-            get { return m_parts.Count; }
+            get { lock (m_parts) { return m_parts.Count; } }
         }
 
         protected Quaternion m_rotation = Quaternion.Identity;
@@ -237,6 +237,7 @@ namespace OpenSim.Region.Framework.Scenes
 
         /// <value>
         /// The parts of this scene object group.  You must lock this property before using it.
+        /// If you want to know the number of children, consider using the PrimCount property instead
         /// </value>
         public Dictionary<UUID, SceneObjectPart> Children
         {
@@ -298,6 +299,7 @@ namespace OpenSim.Region.Framework.Scenes
                 {
                     m_scene.CrossPrimGroupIntoNewRegion(val, this, true);
                 }
+                
                 if (RootPart.GetStatusSandbox())
                 {
                     if (Util.GetDistanceTo(RootPart.StatusSandboxPos, value) > 10)
@@ -308,6 +310,7 @@ namespace OpenSim.Region.Framework.Scenes
                         return;
                     }
                 }
+                
                 lock (m_parts)
                 {
                     foreach (SceneObjectPart part in m_parts.Values)
@@ -558,21 +561,23 @@ namespace OpenSim.Region.Framework.Scenes
             if (m_rootPart.LocalId == 0)
                 m_rootPart.LocalId = m_scene.AllocateLocalId();
 
-            // No need to lock here since the object isn't yet in a scene
-            foreach (SceneObjectPart part in m_parts.Values)
+            lock (m_parts)
             {
-                if (Object.ReferenceEquals(part, m_rootPart))
-                {
-                    continue;
-                }
-
-                if (part.LocalId == 0)
+                foreach (SceneObjectPart part in m_parts.Values)
                 {
-                    part.LocalId = m_scene.AllocateLocalId();
+                    if (Object.ReferenceEquals(part, m_rootPart))
+                    {
+                        continue;
+                    }
+    
+                    if (part.LocalId == 0)
+                    {
+                        part.LocalId = m_scene.AllocateLocalId();
+                    }
+    
+                    part.ParentID = m_rootPart.LocalId;
+                    //m_log.DebugFormat("[SCENE]: Given local id {0} to part {1}, linknum {2}, parent {3} {4}", part.LocalId, part.UUID, part.LinkNum, part.ParentID, part.ParentUUID);
                 }
-
-                part.ParentID = m_rootPart.LocalId;
-                //m_log.DebugFormat("[SCENE]: Given local id {0} to part {1}, linknum {2}, parent {3} {4}", part.LocalId, part.UUID, part.LinkNum, part.ParentID, part.ParentUUID);
             }
             
             ApplyPhysics(m_scene.m_physicalPrim);
@@ -670,7 +675,7 @@ namespace OpenSim.Region.Framework.Scenes
             minY = 256f;
             minZ = 8192f;
 
-            lock(m_parts);
+            lock(m_parts)
             {
                 foreach (SceneObjectPart part in m_parts.Values)
                 {
@@ -995,9 +1000,12 @@ namespace OpenSim.Region.Framework.Scenes
                 m_rootPart.AttachedAvatar = agentID;
 
                 //Anakin Lohner bug #3839 
-                foreach (SceneObjectPart p in m_parts.Values)
+                lock (m_parts)
                 {
-                    p.AttachedAvatar = agentID;
+                    foreach (SceneObjectPart p in m_parts.Values)
+                    {
+                        p.AttachedAvatar = agentID;
+                    }
                 }
 
                 if (m_rootPart.PhysActor != null)
@@ -1065,10 +1073,14 @@ namespace OpenSim.Region.Framework.Scenes
 
             AbsolutePosition = detachedpos;
             m_rootPart.AttachedAvatar = UUID.Zero;
-            //Anakin Lohner bug #3839 
-            foreach (SceneObjectPart p in m_parts.Values)
+            
+            //Anakin Lohner bug #3839            
+            lock (m_parts)
             {
-                p.AttachedAvatar = UUID.Zero;
+                foreach (SceneObjectPart p in m_parts.Values)
+                {
+                    p.AttachedAvatar = UUID.Zero;
+                }
             }
 
             m_rootPart.SetParentLocalId(0);
@@ -1094,10 +1106,14 @@ namespace OpenSim.Region.Framework.Scenes
             }
 
             m_rootPart.AttachedAvatar = UUID.Zero;
+            
             //Anakin Lohner bug #3839 
-            foreach (SceneObjectPart p in m_parts.Values)
+            lock (m_parts)
             {
-                p.AttachedAvatar = UUID.Zero;
+                foreach (SceneObjectPart p in m_parts.Values)
+                {
+                    p.AttachedAvatar = UUID.Zero;
+                }
             }
 
             m_rootPart.SetParentLocalId(0);
@@ -1160,9 +1176,8 @@ namespace OpenSim.Region.Framework.Scenes
                 part.ParentID = 0;
             part.LinkNum = 0;
             
-            // No locking required since the SOG should not be in the scene yet - one can't change root parts after
-            // the scene object has been attached to the scene
-            m_parts.Add(m_rootPart.UUID, m_rootPart);
+            lock (m_parts)
+                m_parts.Add(m_rootPart.UUID, m_rootPart);
         }
 
         /// <summary>
@@ -1625,7 +1640,7 @@ namespace OpenSim.Region.Framework.Scenes
         }
 
         /// <summary>
-        ///
+        /// Copy the given part as the root part of this scene object.
         /// </summary>
         /// <param name="part"></param>
         /// <param name="cAgentID"></param>
@@ -1882,11 +1897,12 @@ namespace OpenSim.Region.Framework.Scenes
         /// <param name="cGroupID"></param>
         public SceneObjectPart CopyPart(SceneObjectPart part, UUID cAgentID, UUID cGroupID, bool userExposed)
         {
-            SceneObjectPart newPart = part.Copy(m_scene.AllocateLocalId(), OwnerID, GroupID, m_parts.Count, userExposed);
-            newPart.SetParent(this);
-
+            SceneObjectPart newPart = null;
+            
             lock (m_parts)
-            {
+            {            
+                newPart = part.Copy(m_scene.AllocateLocalId(), OwnerID, GroupID, m_parts.Count, userExposed);
+                newPart.SetParent(this);
                 m_parts.Add(newPart.UUID, newPart);
             }
 
@@ -1903,14 +1919,15 @@ namespace OpenSim.Region.Framework.Scenes
         /// </summary>
         public void ResetIDs()
         {
-            // As this is only ever called for prims which are not currently part of the scene (and hence
-            // not accessible by clients), there should be no need to lock
-            List<SceneObjectPart> partsList = new List<SceneObjectPart>(m_parts.Values);
-            m_parts.Clear();
-            foreach (SceneObjectPart part in partsList)
+            lock (m_parts)
             {
-                part.ResetIDs(part.LinkNum); // Don't change link nums
-                m_parts.Add(part.UUID, part);
+                List<SceneObjectPart> partsList = new List<SceneObjectPart>(m_parts.Values);
+                m_parts.Clear();
+                foreach (SceneObjectPart part in partsList)
+                {
+                    part.ResetIDs(part.LinkNum); // Don't change link nums
+                    m_parts.Add(part.UUID, part);
+                }
             }
         }
 
@@ -2136,10 +2153,15 @@ namespace OpenSim.Region.Framework.Scenes
         public SceneObjectPart GetChildPart(UUID primID)
         {
             SceneObjectPart childPart = null;
-            if (m_parts.ContainsKey(primID))
+            
+            lock (m_parts)
             {
-                childPart = m_parts[primID];
+                if (m_parts.ContainsKey(primID))
+                {
+                    childPart = m_parts[primID];
+                }
             }
+            
             return childPart;
         }
 
@@ -2174,9 +2196,10 @@ namespace OpenSim.Region.Framework.Scenes
         /// <returns></returns>
         public bool HasChildPrim(UUID primID)
         {
-            if (m_parts.ContainsKey(primID))
+            lock (m_parts)
             {
-                return true;
+                if (m_parts.ContainsKey(primID))
+                    return true;
             }
 
             return false;
@@ -2370,17 +2393,19 @@ namespace OpenSim.Region.Framework.Scenes
             lock (m_parts)
             {
                 m_parts.Remove(linkPart.UUID);
-            }
-
-            if (m_parts.Count == 1 && RootPart != null) //Single prim is left
-                RootPart.LinkNum = 0;
-            else
-            {
-                foreach (SceneObjectPart p in m_parts.Values)
+                
+                if (m_parts.Count == 1 && RootPart != null) //Single prim is left
                 {
-                    if (p.LinkNum > linkPart.LinkNum)
-                        p.LinkNum--;
+                    RootPart.LinkNum = 0;
                 }
+                else
+                {
+                    foreach (SceneObjectPart p in m_parts.Values)
+                    {
+                        if (p.LinkNum > linkPart.LinkNum)
+                            p.LinkNum--;
+                    }
+                }                
             }
 
             linkPart.ParentID = 0;
@@ -2762,9 +2787,11 @@ namespace OpenSim.Region.Framework.Scenes
         public void UpdatePermissions(UUID AgentID, byte field, uint localID,
                 uint mask, byte addRemTF)
         {
-            foreach (SceneObjectPart part in m_parts.Values)
-                part.UpdatePermissions(AgentID, field, localID, mask,
-                        addRemTF);
+            lock (m_parts)
+            {
+                foreach (SceneObjectPart part in m_parts.Values)
+                    part.UpdatePermissions(AgentID, field, localID, mask, addRemTF);
+            }
 
             HasGroupChanged = true;
         }