From e901253b49668f1e1d8f135f2d05aaec7baad40a Mon Sep 17 00:00:00 2001 From: Justin Clark-Casey (justincc) Date: Wed, 17 Dec 2014 00:21:58 +0000 Subject: Fix recent regression where a race condition meant SP.MakeRootAgent() would sometimes look to start attachment scripts before ETM.HandleIncomingSceneObject() had added them. Probably a regression since ghosts branch merge on Nov 26 2014 --- .../Avatar/Attachments/AttachmentsModule.cs | 16 ++++-- OpenSim/Region/Framework/Scenes/ScenePresence.cs | 61 ++++++++++------------ 2 files changed, 40 insertions(+), 37 deletions(-) diff --git a/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs b/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs index ff7a062..4af4ddb 100644 --- a/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs +++ b/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs @@ -341,11 +341,13 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments if (!Enabled) return; - if (DebugLevel > 0) - m_log.DebugFormat("[ATTACHMENTS MODULE]: Saving changed attachments for {0}", sp.Name); - List attachments = sp.GetAttachments(); + if (DebugLevel > 0) + m_log.DebugFormat( + "[ATTACHMENTS MODULE]: Saving for {0} attachments for {1} in {2}", + attachments.Count, sp.Name, m_scene.Name); + if (attachments.Count <= 0) return; @@ -359,6 +361,10 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments // 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); + +// m_log.DebugFormat( +// "[ATTACHMENTS MODULE]: For object {0} for {1} in {2} got saved state {3}", +// so.Name, sp.Name, m_scene.Name, scriptStates[so]); } lock (sp.AttachmentsSyncLock) @@ -826,8 +832,8 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments { if (DebugLevel > 0) m_log.DebugFormat( - "[ATTACHMENTS MODULE]: Adding attachment {0} to avatar {1} in pt {2} pos {3} {4}", - so.Name, sp.Name, attachmentpoint, attachOffset, so.RootPart.AttachedPos); + "[ATTACHMENTS MODULE]: Adding attachment {0} to avatar {1} at pt {2} pos {3} {4} in {5}", + so.Name, sp.Name, attachmentpoint, attachOffset, so.RootPart.AttachedPos, m_scene.Name); // Remove from database and parcel prim count m_scene.DeleteFromStorage(so.UUID); diff --git a/OpenSim/Region/Framework/Scenes/ScenePresence.cs b/OpenSim/Region/Framework/Scenes/ScenePresence.cs index 93dfd00..0414f89 100644 --- a/OpenSim/Region/Framework/Scenes/ScenePresence.cs +++ b/OpenSim/Region/Framework/Scenes/ScenePresence.cs @@ -1228,45 +1228,27 @@ namespace OpenSim.Region.Framework.Scenes m_scene.SwapRootAgentCount(false); - // The initial login scene presence is already root when it gets here - // and it has already rezzed the attachments and started their scripts. - // We do the following only for non-login agents, because their scripts - // haven't started yet. - if (PresenceType == PresenceType.Npc || IsRealLogin(m_teleportFlags)) - { - // Viewers which have a current outfit folder will actually rez their own attachments. However, - // viewers without (e.g. v1 viewers) will not, so we still need to make this call. - if (Scene.AttachmentsModule != null) + if (Scene.AttachmentsModule != null) + { + // The initial login scene presence is already root when it gets here + // and it has already rezzed the attachments and started their scripts. + // We do the following only for non-login agents, because their scripts + // haven't started yet. + if (PresenceType == PresenceType.Npc || IsRealLogin(m_teleportFlags)) { + // Viewers which have a current outfit folder will actually rez their own attachments. However, + // viewers without (e.g. v1 viewers) will not, so we still need to make this call. WorkManager.RunJob( "RezAttachments", o => Scene.AttachmentsModule.RezAttachments(this), null, string.Format("Rez attachments for {0} in {1}", Name, Scene.Name)); } - } - else - { - // We need to restart scripts here so that they receive the correct changed events (CHANGED_TELEPORT - // and CHANGED_REGION) when the attachments have been rezzed in the new region. This cannot currently - // be done in AttachmentsModule.CopyAttachments(AgentData ad, IScenePresence sp) itself since we are - // not transporting the required data. - // - // We must take a copy of the attachments list here (rather than locking) to avoid a deadlock where a script in one of - // the attachments may start processing an event (which locks ScriptInstance.m_Script) that then calls a method here - // which needs to lock m_attachments. ResumeScripts() needs to take a ScriptInstance.m_Script lock to try to unset the Suspend status. - // - // FIXME: In theory, this deadlock should not arise since scripts should not be processing events until ResumeScripts(). - // But XEngine starts all scripts unsuspended. Starting them suspended will not currently work because script rezzing - // is placed in an asynchronous queue in XEngine and so the ResumeScripts() call will almost certainly execute before the - // script is rezzed. This means the ResumeScripts() does absolutely nothing when using XEngine. - List attachments = GetAttachments(); - - if (attachments.Count > 0) + else { WorkManager.RunJob( - "StartAttachmentScripts", - o => RestartAttachmentScripts(attachments), + "StartAttachmentScripts", + o => RestartAttachmentScripts(), null, string.Format("Start attachment scripts for {0} in {1}", Name, Scene.Name), true); @@ -1292,10 +1274,25 @@ namespace OpenSim.Region.Framework.Scenes return true; } - private void RestartAttachmentScripts(List attachments) + private void RestartAttachmentScripts() { + // We need to restart scripts here so that they receive the correct changed events (CHANGED_TELEPORT + // and CHANGED_REGION) when the attachments have been rezzed in the new region. This cannot currently + // be done in AttachmentsModule.CopyAttachments(AgentData ad, IScenePresence sp) itself since we are + // not transporting the required data. + // + // We must take a copy of the attachments list here (rather than locking) to avoid a deadlock where a script in one of + // the attachments may start processing an event (which locks ScriptInstance.m_Script) that then calls a method here + // which needs to lock m_attachments. ResumeScripts() needs to take a ScriptInstance.m_Script lock to try to unset the Suspend status. + // + // FIXME: In theory, this deadlock should not arise since scripts should not be processing events until ResumeScripts(). + // But XEngine starts all scripts unsuspended. Starting them suspended will not currently work because script rezzing + // is placed in an asynchronous queue in XEngine and so the ResumeScripts() call will almost certainly execute before the + // script is rezzed. This means the ResumeScripts() does absolutely nothing when using XEngine. + List attachments = GetAttachments(); + m_log.DebugFormat( - "[SCENE PRESENCE]: Restarting scripts in attachments for {0} in {1}", Name, Scene.Name); + "[SCENE PRESENCE]: Restarting scripts in {0} attachments for {1} in {2}", attachments.Count, Name, Scene.Name); // Resume scripts foreach (SceneObjectGroup sog in attachments) -- cgit v1.1