Make SP.Attachments available as sp.GetAttachments() instead.

The approach here, as in other parts of OpenSim, is to return a copy of the list rather than the attachments list itself
This prevents callers from forgetting to lock the list when they read it, as was happening in various parts of the codebase.
It also improves liveness.
This might improve attachment anomolies when performing region crossings.

4 files changed, 52 insertions, 50 deletions

diff --git a/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs b/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs
index f4bc495..9e5ce8f 100644
--- a/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs
+++ b/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs
@@ -136,10 +136,7 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments
 
         public void SaveChangedAttachments(IScenePresence sp)
         {
-            // Need to copy this list because DetachToInventoryPrep mods it
-            List<SceneObjectGroup> attachments = new List<SceneObjectGroup>(sp.Attachments);
-
-            foreach (SceneObjectGroup grp in attachments)
+            foreach (SceneObjectGroup grp in sp.GetAttachments())
             {
                 if (grp.HasGroupChanged) // Resizer scripts?
                 {
@@ -273,14 +270,12 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments
 
             // Remove any previous attachments
             UUID itemID = UUID.Zero;
-            foreach (SceneObjectGroup grp in sp.Attachments)
-            {
-                if (grp.AttachmentPoint == attachmentPt)
-                {
-                    itemID = grp.GetFromItemID();
-                    break;
-                }
-            }
+
+            List<SceneObjectGroup> attachments = sp.GetAttachments(attachmentPt);
+
+            // At the moment we can only deal with a single attachment
+            if (attachments.Count != 0)
+                itemID = attachments[0].GetFromItemID();
 
             if (itemID != UUID.Zero)
                 DetachSingleAttachmentToInv(itemID, sp);
diff --git a/OpenSim/Region/CoreModules/Avatar/Attachments/Tests/AttachmentsModuleTests.cs b/OpenSim/Region/CoreModules/Avatar/Attachments/Tests/AttachmentsModuleTests.cs
index b0146a1..363e258 100644
--- a/OpenSim/Region/CoreModules/Avatar/Attachments/Tests/AttachmentsModuleTests.cs
+++ b/OpenSim/Region/CoreModules/Avatar/Attachments/Tests/AttachmentsModuleTests.cs
@@ -106,7 +106,7 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments.Tests
 
             // Check status on scene presence
             Assert.That(m_presence.HasAttachments(), Is.True);
-            List<SceneObjectGroup> attachments = m_presence.Attachments;
+            List<SceneObjectGroup> attachments = m_presence.GetAttachments();
             Assert.That(attachments.Count, Is.EqualTo(1));
             SceneObjectGroup attSo = attachments[0];
             Assert.That(attSo.Name, Is.EqualTo(attName));
@@ -140,7 +140,7 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments.Tests
 
             // Check scene presence status
             Assert.That(m_presence.HasAttachments(), Is.True);
-            List<SceneObjectGroup> attachments = m_presence.Attachments;
+            List<SceneObjectGroup> attachments = m_presence.GetAttachments();
             Assert.That(attachments.Count, Is.EqualTo(1));
             SceneObjectGroup attSo = attachments[0];
             Assert.That(attSo.Name, Is.EqualTo(attName));
@@ -174,7 +174,7 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments.Tests
 
             // Check scene presence status
             Assert.That(m_presence.HasAttachments(), Is.False);
-            List<SceneObjectGroup> attachments = m_presence.Attachments;
+            List<SceneObjectGroup> attachments = m_presence.GetAttachments();
             Assert.That(attachments.Count, Is.EqualTo(0));
 
             // Check appearance status
@@ -208,7 +208,7 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments.Tests
 
             // Check status on scene presence
             Assert.That(m_presence.HasAttachments(), Is.False);
-            List<SceneObjectGroup> attachments = m_presence.Attachments;
+            List<SceneObjectGroup> attachments = m_presence.GetAttachments();
             Assert.That(attachments.Count, Is.EqualTo(0));
 
             // Check item status
@@ -237,7 +237,7 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments.Tests
             ScenePresence presence = SceneHelpers.AddScenePresence(scene, acd);
 
             Assert.That(presence.HasAttachments(), Is.True);
-            List<SceneObjectGroup> attachments = presence.Attachments;
+            List<SceneObjectGroup> attachments = presence.GetAttachments();
 
             Assert.That(attachments.Count, Is.EqualTo(1));
             SceneObjectGroup attSo = attachments[0];
diff --git a/OpenSim/Region/CoreModules/Framework/EntityTransfer/EntityTransferModule.cs b/OpenSim/Region/CoreModules/Framework/EntityTransfer/EntityTransferModule.cs
index 7963e53..c24cc17 100644
--- a/OpenSim/Region/CoreModules/Framework/EntityTransfer/EntityTransferModule.cs
+++ b/OpenSim/Region/CoreModules/Framework/EntityTransfer/EntityTransferModule.cs
@@ -208,7 +208,7 @@ namespace OpenSim.Region.CoreModules.Framework.EntityTransfer
                     sp.ControllingClient.SendLocalTeleport(position, lookAt, teleportFlags);
                     sp.Teleport(position);
 
-                    foreach (SceneObjectGroup grp in sp.Attachments)
+                    foreach (SceneObjectGroup grp in sp.GetAttachments())
                         sp.Scene.EventManager.TriggerOnScriptChangedEvent(grp.LocalId, (uint)Changed.TELEPORT);
                 }
                 else // Another region possibly in another simulator
@@ -559,11 +559,11 @@ namespace OpenSim.Region.CoreModules.Framework.EntityTransfer
 
         protected virtual void AgentHasMovedAway(ScenePresence sp, bool logout)
         {
-            foreach (SceneObjectGroup sop in sp.Attachments)
+            foreach (SceneObjectGroup sop in sp.GetAttachments())
             {
                 sop.Scene.DeleteSceneObject(sop, true);
             }
-            sp.Attachments.Clear();
+            sp.ClearAttachments();
         }
 
         protected void KillEntity(Scene scene, uint localID)
@@ -1764,34 +1764,33 @@ namespace OpenSim.Region.CoreModules.Framework.EntityTransfer
 
         protected bool CrossAttachmentsIntoNewRegion(GridRegion destination, ScenePresence sp, bool silent)
         {
-            List<SceneObjectGroup> m_attachments = sp.Attachments;
-            lock (m_attachments)
+            List<SceneObjectGroup> m_attachments = sp.GetAttachments();
+
+            // Validate
+            foreach (SceneObjectGroup gobj in m_attachments)
             {
-                // Validate
-                foreach (SceneObjectGroup gobj in m_attachments)
-                {
-                    if (gobj == null || gobj.IsDeleted)
-                        return false;
-                }
+                if (gobj == null || gobj.IsDeleted)
+                    return false;
+            }
 
-                foreach (SceneObjectGroup gobj in m_attachments)
+            foreach (SceneObjectGroup gobj in m_attachments)
+            {
+                // If the prim group is null then something must have happened to it!
+                if (gobj != null && gobj.RootPart != null)
                 {
-                    // If the prim group is null then something must have happened to it!
-                    if (gobj != null && gobj.RootPart != null)
-                    {
-                        // Set the parent localID to 0 so it transfers over properly.
-                        gobj.RootPart.SetParentLocalId(0);
-                        gobj.AbsolutePosition = gobj.RootPart.AttachedPos;
-                        gobj.IsAttachment = false;
-                        //gobj.RootPart.LastOwnerID = gobj.GetFromAssetID();
-                        m_log.DebugFormat("[ENTITY TRANSFER MODULE]: Sending attachment {0} to region {1}", gobj.UUID, destination.RegionName);
-                        CrossPrimGroupIntoNewRegion(destination, gobj, silent);
-                    }
+                    // Set the parent localID to 0 so it transfers over properly.
+                    gobj.RootPart.SetParentLocalId(0);
+                    gobj.AbsolutePosition = gobj.RootPart.AttachedPos;
+                    gobj.IsAttachment = false;
+                    //gobj.RootPart.LastOwnerID = gobj.GetFromAssetID();
+                    m_log.DebugFormat("[ENTITY TRANSFER MODULE]: Sending attachment {0} to region {1}", gobj.UUID, destination.RegionName);
+                    CrossPrimGroupIntoNewRegion(destination, gobj, silent);
                 }
-                m_attachments.Clear();
-
-                return true;
             }
+
+            sp.ClearAttachments();
+
+            return true;
         }
 
         #endregion
@@ -1840,7 +1839,9 @@ namespace OpenSim.Region.CoreModules.Framework.EntityTransfer
             int i = 0;
             if (sp.InTransitScriptStates.Count > 0)
             {
-                sp.Attachments.ForEach(delegate(SceneObjectGroup sog)
+                List<SceneObjectGroup> attachments = sp.GetAttachments();
+
+                foreach (SceneObjectGroup sog in attachments)
                 {
                     if (i < sp.InTransitScriptStates.Count)
                     {
@@ -1849,8 +1850,10 @@ namespace OpenSim.Region.CoreModules.Framework.EntityTransfer
                         sog.ResumeScripts();
                     }
                     else
-                        m_log.ErrorFormat("[ENTITY TRANSFER MODULE]: InTransitScriptStates.Count={0} smaller than Attachments.Count={1}", sp.InTransitScriptStates.Count, sp.Attachments.Count);
-                });
+                        m_log.ErrorFormat(
+                            "[ENTITY TRANSFER MODULE]: InTransitScriptStates.Count={0} smaller than Attachments.Count={1}",
+                            sp.InTransitScriptStates.Count, attachments.Count);
+                }
 
                 sp.InTransitScriptStates.Clear();
             }
diff --git a/OpenSim/Region/CoreModules/Scripting/WorldComm/WorldCommModule.cs b/OpenSim/Region/CoreModules/Scripting/WorldComm/WorldCommModule.cs
index 22352f5..057500c 100644
--- a/OpenSim/Region/CoreModules/Scripting/WorldComm/WorldCommModule.cs
+++ b/OpenSim/Region/CoreModules/Scripting/WorldComm/WorldCommModule.cs
@@ -283,7 +283,7 @@ namespace OpenSim.Region.CoreModules.Scripting.WorldComm
         }
 
         /// <summary>
-        /// Delivers the message to.
+        /// Delivers the message to a scene entity.
         /// </summary>
         /// <param name='target'>
         /// Target.
@@ -314,17 +314,19 @@ namespace OpenSim.Region.CoreModules.Scripting.WorldComm
                     m_scene.SimChatBroadcast(Utils.StringToBytes(msg), ChatTypeEnum.Owner, 0, pos, name, id, false);
                 }
 
-                List<SceneObjectGroup> attachments = sp.Attachments;
+                List<SceneObjectGroup> attachments = sp.GetAttachments();
+
                 // Nothing left to do
                 if (attachments == null)
                     return true;
 
                 // Get uuid of attachments
                 List<UUID> targets = new List<UUID>();
-                foreach ( SceneObjectGroup sog in attachments )
+                foreach (SceneObjectGroup sog in attachments)
                 {
                     targets.Add(sog.UUID);
                 }
+
                 // Need to check each attachment
                 foreach (ListenerInfo li in m_listenerManager.GetListeners(UUID.Zero, channel, name, id, msg))
                 {
@@ -334,9 +336,10 @@ namespace OpenSim.Region.CoreModules.Scripting.WorldComm
                     if (m_scene.GetSceneObjectPart(li.GetHostID()) == null)
                         continue;
 
-                    if ( targets.Contains(li.GetHostID()))
+                    if (targets.Contains(li.GetHostID()))
                         QueueMessage(new ListenerInfo(li, name, id, msg));
                 }
+
                 return true;
             }
 
@@ -363,6 +366,7 @@ namespace OpenSim.Region.CoreModules.Scripting.WorldComm
                     break;
                 }
             }
+            
             return true;
         }