From ccd6f443e1092cb410f565e921f7cf4dd8cd2dac Mon Sep 17 00:00:00 2001
From: Justin Clark-Casey (justincc)
Date: Tue, 5 Mar 2013 23:47:36 +0000
Subject: Get attachment script state before taking sp.AttachmentsSyncLock() to
avoid race conditions between closing agents and scripts that may be doing
attachment manipulation.
This is in an effort to resolve http://opensimulator.org/mantis/view.php?id=6557
---
.../Avatar/Attachments/AttachmentsModule.cs | 301 +++++++++++----------
1 file changed, 161 insertions(+), 140 deletions(-)
diff --git a/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs b/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs
index 8a3eeaa..3ccf9f4 100644
--- a/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs
+++ b/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs
@@ -241,12 +241,27 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments
// m_log.DebugFormat("[ATTACHMENTS MODULE]: Saving changed attachments for {0}", sp.Name);
+ List<SceneObjectGroup> attachments = sp.GetAttachments();
+
+ if (attachments.Count <= 0)
+ return;
+
+ Dictionary<SceneObjectGroup, string> scriptStates = new Dictionary<SceneObjectGroup, string>();
+
+ foreach (SceneObjectGroup so in attachments)
+ {
+ // Scripts MUST be snapshotted before the object is
+ // removed from the scene because doing otherwise will
+ // clobber the run flag
+ // This must be done outside the sp.AttachmentSyncLock so that there is no risk of a deadlock from
+ // scripts performing attachment operations at the same time. Getting object states stops the scripts.
+ scriptStates[so] = PrepareScriptInstanceForSave(so, false);
+ }
+
lock (sp.AttachmentsSyncLock)
{
- foreach (SceneObjectGroup so in sp.GetAttachments())
- {
- UpdateDetachedObject(sp, so);
- }
+ foreach (SceneObjectGroup so in attachments)
+ UpdateDetachedObject(sp, so, scriptStates[so]);
sp.ClearAttachments();
}
@@ -285,32 +300,40 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments
private bool AttachObjectInternal(IScenePresence sp, SceneObjectGroup group, uint attachmentPt, bool silent, bool temp)
{
- lock (sp.AttachmentsSyncLock)
- {
// m_log.DebugFormat(
// "[ATTACHMENTS MODULE]: Attaching object {0} {1} to {2} point {3} from ground (silent = {4})",
// group.Name, group.LocalId, sp.Name, attachmentPt, silent);
- if (group.GetSittingAvatarsCount() != 0)
- {
+ if (group.GetSittingAvatarsCount() != 0)
+ {
// m_log.WarnFormat(
// "[ATTACHMENTS MODULE]: Ignoring request to attach {0} {1} to {2} on {3} since {4} avatars are still sitting on it",
// group.Name, group.LocalId, sp.Name, attachmentPt, group.GetSittingAvatarsCount());
-
- return false;
- }
-
- if (sp.GetAttachments(attachmentPt).Contains(group))
- {
- // m_log.WarnFormat(
- // "[ATTACHMENTS MODULE]: Ignoring request to attach {0} {1} to {2} on {3} since it's already attached",
- // group.Name, group.LocalId, sp.Name, AttachmentPt);
-
- return false;
- }
-
+
+ return false;
+ }
+
+ if (sp.GetAttachments(attachmentPt).Contains(group))
+ {
+// m_log.WarnFormat(
+// "[ATTACHMENTS MODULE]: Ignoring request to attach {0} {1} to {2} on {3} since it's already attached",
+// group.Name, group.LocalId, sp.Name, AttachmentPt);
+
+ return false;
+ }
+
+ // Remove any previous attachments
+ List<SceneObjectGroup> existingAttachments = sp.GetAttachments(attachmentPt);
+ string existingAttachmentScriptState = null;
+
+ // At the moment we can only deal with a single attachment
+ if (existingAttachments.Count != 0 && existingAttachments[0].FromItemID != UUID.Zero)
+ DetachSingleAttachmentToInv(sp, group);
+
+ lock (sp.AttachmentsSyncLock)
+ {
Vector3 attachPos = group.AbsolutePosition;
-
+
// TODO: this short circuits multiple attachments functionality in LL viewer 2.1+ and should
// be removed when that functionality is implemented in opensim
attachmentPt &= 0x7f;
@@ -322,14 +345,14 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments
{
attachPos = Vector3.Zero;
}
-
+
// AttachmentPt 0 means the client chose to 'wear' the attachment.
if (attachmentPt == 0)
{
// Check object for stored attachment point
attachmentPt = group.AttachmentPoint;
}
-
+
// if we still didn't find a suitable attachment point.......
if (attachmentPt == 0)
{
@@ -337,13 +360,13 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments
attachmentPt = (uint)AttachmentPoint.LeftHand;
attachPos = Vector3.Zero;
}
-
+
group.AttachmentPoint = attachmentPt;
group.AbsolutePosition = attachPos;
if (sp.PresenceType != PresenceType.Npc)
UpdateUserInventoryWithAttachment(sp, group, attachmentPt, temp);
-
+
AttachToAgent(sp, group, attachmentPt, attachPos, silent);
}
@@ -352,21 +375,6 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments
private void UpdateUserInventoryWithAttachment(IScenePresence sp, SceneObjectGroup group, uint attachmentPt, bool temp)
{
- // Remove any previous attachments
- List<SceneObjectGroup> attachments = sp.GetAttachments(attachmentPt);
-
- // At the moment we can only deal with a single attachment
- if (attachments.Count != 0)
- {
- if (attachments[0].FromItemID != UUID.Zero)
- DetachSingleAttachmentToInvInternal(sp, attachments[0]);
- // Error logging commented because UUID.Zero now means temp attachment
-// else
-// m_log.WarnFormat(
-// "[ATTACHMENTS MODULE]: When detaching existing attachment {0} {1} at point {2} to make way for {3} {4} for {5}, couldn't find the associated item ID to adjust inventory attachment record!",
-// attachments[0].Name, attachments[0].LocalId, attachmentPt, group.Name, group.LocalId, sp.Name);
- }
-
// Add the new attachment to inventory if we don't already have it.
if (!temp)
{
@@ -426,12 +434,10 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments
return;
// m_log.DebugFormat("[ATTACHMENTS MODULE]: Rezzing multiple attachments from inventory for {0}", sp.Name);
- lock (sp.AttachmentsSyncLock)
+
+ foreach (KeyValuePair<UUID, uint> rez in rezlist)
{
- foreach (KeyValuePair<UUID, uint> rez in rezlist)
- {
- RezSingleAttachmentFromInventory(sp, rez.Key, rez.Value);
- }
+ RezSingleAttachmentFromInventory(sp, rez.Key, rez.Value);
}
}
@@ -511,25 +517,33 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments
public void DetachSingleAttachmentToInv(IScenePresence sp, SceneObjectGroup so)
{
+ if (so.AttachedAvatar != sp.UUID)
+ {
+ m_log.WarnFormat(
+ "[ATTACHMENTS MODULE]: Tried to detach object {0} from {1} {2} but attached avatar id was {3} in {4}",
+ so.Name, sp.Name, sp.UUID, so.AttachedAvatar, m_scene.RegionInfo.RegionName);
+
+ return;
+ }
+
+ // Scripts MUST be snapshotted before the object is
+ // removed from the scene because doing otherwise will
+ // clobber the run flag
+ // This must be done outside the sp.AttachmentSyncLock so that there is no risk of a deadlock from
+ // scripts performing attachment operations at the same time. Getting object states stops the scripts.
+ string scriptedState = PrepareScriptInstanceForSave(so, true);
+
lock (sp.AttachmentsSyncLock)
{
// Save avatar attachment information
// m_log.Debug("[ATTACHMENTS MODULE]: Detaching from UserID: " + sp.UUID + ", ItemID: " + itemID);
- if (so.AttachedAvatar != sp.UUID)
- {
- m_log.WarnFormat(
- "[ATTACHMENTS MODULE]: Tried to detach object {0} from {1} {2} but attached avatar id was {3} in {4}",
- so.Name, sp.Name, sp.UUID, so.AttachedAvatar, m_scene.RegionInfo.RegionName);
-
- return;
- }
-
bool changed = sp.Appearance.DetachAttachment(so.FromItemID);
if (changed && m_scene.AvatarFactory != null)
m_scene.AvatarFactory.QueueAppearanceSave(sp.UUID);
- DetachSingleAttachmentToInvInternal(sp, so);
+ sp.RemoveAttachment(so);
+ UpdateDetachedObject(sp, so, scriptedState);
}
}
@@ -739,8 +753,27 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments
return newItem;
}
- private string GetObjectScriptStates(SceneObjectGroup grp)
+ /// <summary>
+ /// Prepares the script instance for save.
+ /// </summary>
+ /// <remarks>
+ /// This involves triggering the detach event and getting the script state (which also stops the script)
+ /// This MUST be done outside sp.AttachmentsSyncLock, since otherwise there is a chance of deadlock if a
+ /// running script is performing attachment operations.
+ /// </remarks>
+ /// <returns>
+ /// The script state ready for persistence.
+ /// </returns>
+ /// <param name='grp'>
+ /// </param>
+ /// <param name='fireDetachEvent'>
+ /// If true, then fire the script event before we save its state.
+ /// </param>
+ private string PrepareScriptInstanceForSave(SceneObjectGroup grp, bool fireDetachEvent)
{
+ if (fireDetachEvent)
+ m_scene.EventManager.TriggerOnAttach(grp.LocalId, grp.FromItemID, UUID.Zero);
+
using (StringWriter sw = new StringWriter())
{
using (XmlTextWriter writer = new XmlTextWriter(sw))
@@ -752,7 +785,7 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments
}
}
- private void UpdateDetachedObject(IScenePresence sp, SceneObjectGroup so)
+ private void UpdateDetachedObject(IScenePresence sp, SceneObjectGroup so, string scriptedState)
{
// Don't save attachments for HG visitors, it
// messes up their inventory. When a HG visitor logs
@@ -765,11 +798,6 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments
&& (m_scene.UserManagementModule == null
|| m_scene.UserManagementModule.IsLocalGridUser(sp.UUID));
- // Scripts MUST be snapshotted before the object is
- // removed from the scene because doing otherwise will
- // clobber the run flag
- string scriptedState = GetObjectScriptStates(so);
-
// Remove the object from the scene so no more updates
// are sent. Doing this before the below changes will ensure
// updates can't cause "HUD artefacts"
@@ -793,91 +821,87 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments
so.RemoveScriptInstances(true);
}
- private void DetachSingleAttachmentToInvInternal(IScenePresence sp, SceneObjectGroup so)
- {
- // m_log.DebugFormat("[ATTACHMENTS MODULE]: Detaching item {0} to inventory for {1}", itemID, sp.Name);
-
- m_scene.EventManager.TriggerOnAttach(so.LocalId, so.FromItemID, UUID.Zero);
- sp.RemoveAttachment(so);
-
- UpdateDetachedObject(sp, so);
- }
-
private SceneObjectGroup RezSingleAttachmentFromInventoryInternal(
IScenePresence sp, UUID itemID, UUID assetID, uint attachmentPt)
{
if (m_invAccessModule == null)
return null;
- lock (sp.AttachmentsSyncLock)
+ SceneObjectGroup objatt;
+
+ if (itemID != UUID.Zero)
+ objatt = m_invAccessModule.RezObject(sp.ControllingClient,
+ itemID, Vector3.Zero, Vector3.Zero, UUID.Zero, (byte)1, true,
+ false, false, sp.UUID, true);
+ else
+ objatt = m_invAccessModule.RezObject(sp.ControllingClient,
+ null, assetID, Vector3.Zero, Vector3.Zero, UUID.Zero, (byte)1, true,
+ false, false, sp.UUID, true);
+
+ if (objatt == null)
{
- SceneObjectGroup objatt;
+ m_log.WarnFormat(
+ "[ATTACHMENTS MODULE]: Could not retrieve item {0} for attaching to avatar {1} at point {2}",
+ itemID, sp.Name, attachmentPt);
- if (itemID != UUID.Zero)
- objatt = m_invAccessModule.RezObject(sp.ControllingClient,
- itemID, Vector3.Zero, Vector3.Zero, UUID.Zero, (byte)1, true,
- false, false, sp.UUID, true);
- else
- objatt = m_invAccessModule.RezObject(sp.ControllingClient,
- null, assetID, Vector3.Zero, Vector3.Zero, UUID.Zero, (byte)1, true,
- false, false, sp.UUID, true);
+ return null;
+ }
- if (objatt != null)
- {
+ // Remove any previous attachments
+ List<SceneObjectGroup> attachments = sp.GetAttachments(attachmentPt);
+ string previousAttachmentScriptedState = null;
+
+ // At the moment we can only deal with a single attachment
+ if (attachments.Count != 0)
+ DetachSingleAttachmentToInv(sp, attachments[0]);
+
+ lock (sp.AttachmentsSyncLock)
+ {
// m_log.DebugFormat(
// "[ATTACHMENTS MODULE]: Rezzed single object {0} for attachment to {1} on point {2} in {3}",
// objatt.Name, sp.Name, attachmentPt, m_scene.Name);
- // HasGroupChanged is being set from within RezObject. Ideally it would be set by the caller.
- objatt.HasGroupChanged = false;
- bool tainted = false;
- if (attachmentPt != 0 && attachmentPt != objatt.AttachmentPoint)
- tainted = true;
-
- // FIXME: Detect whether it's really likely for AttachObject to throw an exception in the normal
- // course of events. If not, then it's probably not worth trying to recover the situation
- // since this is more likely to trigger further exceptions and confuse later debugging. If
- // exceptions can be thrown in expected error conditions (not NREs) then make this consistent
- // since other normal error conditions will simply return false instead.
- // This will throw if the attachment fails
- try
- {
- AttachObjectInternal(sp, objatt, attachmentPt, false, false);
- }
- catch (Exception e)
- {
- m_log.ErrorFormat(
- "[ATTACHMENTS MODULE]: Failed to attach {0} {1} for {2}, exception {3}{4}",
- objatt.Name, objatt.UUID, sp.Name, e.Message, e.StackTrace);
-
- // Make sure the object doesn't stick around and bail
- sp.RemoveAttachment(objatt);
- m_scene.DeleteSceneObject(objatt, false);
- return null;
- }
+ // HasGroupChanged is being set from within RezObject. Ideally it would be set by the caller.
+ objatt.HasGroupChanged = false;
+ bool tainted = false;
+ if (attachmentPt != 0 && attachmentPt != objatt.AttachmentPoint)
+ tainted = true;
+
+ // FIXME: Detect whether it's really likely for AttachObject to throw an exception in the normal
+ // course of events. If not, then it's probably not worth trying to recover the situation
+ // since this is more likely to trigger further exceptions and confuse later debugging. If
+ // exceptions can be thrown in expected error conditions (not NREs) then make this consistent
+ // since other normal error conditions will simply return false instead.
+ // This will throw if the attachment fails
+ try
+ {
+ AttachObjectInternal(sp, objatt, attachmentPt, false, false);
+ }
+ catch (Exception e)
+ {
+ m_log.ErrorFormat(
+ "[ATTACHMENTS MODULE]: Failed to attach {0} {1} for {2}, exception {3}{4}",
+ objatt.Name, objatt.UUID, sp.Name, e.Message, e.StackTrace);
- if (tainted)
- objatt.HasGroupChanged = true;
+ // Make sure the object doesn't stick around and bail
+ sp.RemoveAttachment(objatt);
+ m_scene.DeleteSceneObject(objatt, false);
+ return null;
+ }
- // Fire after attach, so we don't get messy perms dialogs
- // 4 == AttachedRez
- objatt.CreateScriptInstances(0, true, m_scene.DefaultScriptEngine, 4);
- objatt.ResumeScripts();
+ if (tainted)
+ objatt.HasGroupChanged = true;
- // Do this last so that event listeners have access to all the effects of the attachment
- m_scene.EventManager.TriggerOnAttach(objatt.LocalId, itemID, sp.UUID);
+ // Fire after attach, so we don't get messy perms dialogs
+ // 4 == AttachedRez
+ objatt.CreateScriptInstances(0, true, m_scene.DefaultScriptEngine, 4);
+ objatt.ResumeScripts();
- return objatt;
- }
- else
- {
- m_log.WarnFormat(
- "[ATTACHMENTS MODULE]: Could not retrieve item {0} for attaching to avatar {1} at point {2}",
- itemID, sp.Name, attachmentPt);
- }
- }
+ // Do this last so that event listeners have access to all the effects of the attachment
+ m_scene.EventManager.TriggerOnAttach(objatt.LocalId, itemID, sp.UUID);
- return null;
+ return objatt;
+ }
}
/// <summary>
@@ -1027,17 +1051,14 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments
ScenePresence sp = m_scene.GetScenePresence(remoteClient.AgentId);
if (sp != null)
{
- lock (sp.AttachmentsSyncLock)
+ List<SceneObjectGroup> attachments = sp.GetAttachments();
+
+ foreach (SceneObjectGroup group in attachments)
{
- List<SceneObjectGroup> attachments = sp.GetAttachments();
-
- foreach (SceneObjectGroup group in attachments)
+ if (group.FromItemID == itemID && group.FromItemID != UUID.Zero)
{
- if (group.FromItemID == itemID && group.FromItemID != UUID.Zero)
- {
- DetachSingleAttachmentToInv(sp, group);
- return;
- }
+ DetachSingleAttachmentToInv(sp, group);
+ return;
}
}
}
@@ -1055,4 +1076,4 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments
#endregion
}
-}
+}
\ No newline at end of file
--
cgit v1.1