From eb5ec4a78698a3b1fd6d581c800ac985d5d946fa Mon Sep 17 00:00:00 2001 From: "Justin Clark-Casey (justincc)" Date: Tue, 10 Jul 2012 21:42:51 +0100 Subject: [PATCH 1/5] If a script is being stopped manually, then give the scriptpool thread 1 second to finish normally before forcibly aborting. This is to avoid the worst of the problems in mono 2.6, 2.10 where an aborted thread does not always release all its locks. This very short grace period is identical to the existing behaviour when a script is removed from the scene. --- OpenSim/Region/ScriptEngine/XEngine/XEngine.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/OpenSim/Region/ScriptEngine/XEngine/XEngine.cs b/OpenSim/Region/ScriptEngine/XEngine/XEngine.cs index 7f3bd765f0..efcae94c8d 100644 --- a/OpenSim/Region/ScriptEngine/XEngine/XEngine.cs +++ b/OpenSim/Region/ScriptEngine/XEngine/XEngine.cs @@ -1574,7 +1574,11 @@ namespace OpenSim.Region.ScriptEngine.XEngine { IScriptInstance instance = GetInstance(itemID); if (instance != null) - instance.Stop(0); + { + // Give the script some time to finish processing its last event. Simply aborting the script thread can + // cause issues on mono 2.6, 2.10 and possibly later where locks are not released properly on abort. + instance.Stop(1000); + } } public DetectParams GetDetectParams(UUID itemID, int idx) From f3134b5cf688af9b824880e0221072b24d22f33e Mon Sep 17 00:00:00 2001 From: "Justin Clark-Casey (justincc)" Date: Tue, 10 Jul 2012 22:41:11 +0100 Subject: [PATCH 2/5] When an attachment is detached to inv or derezzed, stop the scripts, update the known item with script state still in the script engine and then remove the scripts. This is to fix a regression starting from 5301648 where attachments had to start being deleted before persistence in order to avoid race conditions with hud update threads. --- .../Avatar/Attachments/AttachmentsModule.cs | 20 +++++-- .../Framework/Interfaces/IEntityInventory.cs | 15 ++++- OpenSim/Region/Framework/Scenes/Scene.cs | 19 +++++- .../Scenes/SceneObjectGroup.Inventory.cs | 10 +++- .../Scenes/SceneObjectPartInventory.cs | 59 ++++++++++++++++++- 5 files changed, 110 insertions(+), 13 deletions(-) diff --git a/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs b/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs index eccf7a6977..efab6ed5ff 100644 --- a/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs +++ b/OpenSim/Region/CoreModules/Avatar/Attachments/AttachmentsModule.cs @@ -211,16 +211,20 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments lock (sp.AttachmentsSyncLock) { - foreach (SceneObjectGroup grp in sp.GetAttachments()) + foreach (SceneObjectGroup so in sp.GetAttachments()) { - grp.Scene.DeleteSceneObject(grp, false); + // We can only remove the script instances from the script engine after we've retrieved their xml state + // when we update the attachment item. + m_scene.DeleteSceneObject(so, false, false); if (saveChanged || saveAllScripted) { - grp.IsAttachment = false; - grp.AbsolutePosition = grp.RootPart.AttachedPos; - UpdateKnownItem(sp, grp, saveAllScripted); + so.IsAttachment = false; + so.AbsolutePosition = so.RootPart.AttachedPos; + UpdateKnownItem(sp, so, saveAllScripted); } + + so.RemoveScriptInstances(true); } sp.ClearAttachments(); @@ -682,7 +686,10 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments m_scene.EventManager.TriggerOnAttach(so.LocalId, so.FromItemID, UUID.Zero); sp.RemoveAttachment(so); - m_scene.DeleteSceneObject(so, false); + + // We can only remove the script instances from the script engine after we've retrieved their xml state + // when we update the attachment item. + m_scene.DeleteSceneObject(so, false, false); // Prepare sog for storage so.AttachedAvatar = UUID.Zero; @@ -691,6 +698,7 @@ namespace OpenSim.Region.CoreModules.Avatar.Attachments so.AbsolutePosition = so.RootPart.AttachedPos; UpdateKnownItem(sp, so, true); + so.RemoveScriptInstances(true); } private SceneObjectGroup RezSingleAttachmentFromInventoryInternal( diff --git a/OpenSim/Region/Framework/Interfaces/IEntityInventory.cs b/OpenSim/Region/Framework/Interfaces/IEntityInventory.cs index 1c9bdce832..8d6284739b 100644 --- a/OpenSim/Region/Framework/Interfaces/IEntityInventory.cs +++ b/OpenSim/Region/Framework/Interfaces/IEntityInventory.cs @@ -92,7 +92,7 @@ namespace OpenSim.Region.Framework.Interfaces void ResumeScripts(); /// - /// Stop all the scripts in this entity. + /// Stop and remove all the scripts in this entity from the scene. /// /// /// Should be true if these scripts are being removed because the scene @@ -100,6 +100,11 @@ namespace OpenSim.Region.Framework.Interfaces /// void RemoveScriptInstances(bool sceneObjectBeingDeleted); + /// + /// Stop all the scripts in this entity. + /// + void StopScriptInstances(); + /// /// Start a script which is in this entity's inventory. /// @@ -129,7 +134,7 @@ namespace OpenSim.Region.Framework.Interfaces bool CreateScriptInstance(UUID itemId, int startParam, bool postOnRez, string engine, int stateSource); /// - /// Stop a script which is in this prim's inventory. + /// Stop and remove a script which is in this prim's inventory from the scene. /// /// /// @@ -138,6 +143,12 @@ namespace OpenSim.Region.Framework.Interfaces /// void RemoveScriptInstance(UUID itemId, bool sceneObjectBeingDeleted); + /// + /// Stop a script which is in this prim's inventory. + /// + /// + void StopScriptInstance(UUID itemId); + /// /// Add an item to this entity's inventory. If an item with the same name already exists, then an alternative /// name is chosen. diff --git a/OpenSim/Region/Framework/Scenes/Scene.cs b/OpenSim/Region/Framework/Scenes/Scene.cs index ec911a52d4..25223b95b3 100644 --- a/OpenSim/Region/Framework/Scenes/Scene.cs +++ b/OpenSim/Region/Framework/Scenes/Scene.cs @@ -2183,13 +2183,30 @@ namespace OpenSim.Region.Framework.Scenes /// /// Synchronously delete the given object from the scene. /// + /// + /// Scripts are also removed. + /// /// Object Id /// Suppress broadcasting changes to other clients. public void DeleteSceneObject(SceneObjectGroup group, bool silent) + { + DeleteSceneObject(group, silent, true); + } + + /// + /// Synchronously delete the given object from the scene. + /// + /// Object Id + /// Suppress broadcasting changes to other clients. + /// If true, then scripts are removed. If false, then they are only stopped. + public void DeleteSceneObject(SceneObjectGroup group, bool silent, bool removeScripts) { // m_log.DebugFormat("[SCENE]: Deleting scene object {0} {1}", group.Name, group.UUID); - group.RemoveScriptInstances(true); + if (removeScripts) + group.RemoveScriptInstances(true); + else + group.StopScriptInstances(); SceneObjectPart[] partList = group.Parts; diff --git a/OpenSim/Region/Framework/Scenes/SceneObjectGroup.Inventory.cs b/OpenSim/Region/Framework/Scenes/SceneObjectGroup.Inventory.cs index 2866b546eb..ddf5da0ba4 100644 --- a/OpenSim/Region/Framework/Scenes/SceneObjectGroup.Inventory.cs +++ b/OpenSim/Region/Framework/Scenes/SceneObjectGroup.Inventory.cs @@ -79,7 +79,7 @@ namespace OpenSim.Region.Framework.Scenes } /// - /// Stop the scripts contained in all the prims in this group + /// Stop and remove the scripts contained in all the prims in this group /// /// /// Should be true if these scripts are being removed because the scene @@ -92,6 +92,14 @@ namespace OpenSim.Region.Framework.Scenes parts[i].Inventory.RemoveScriptInstances(sceneObjectBeingDeleted); } + /// + /// Stop the scripts contained in all the prims in this group + /// + public void StopScriptInstances() + { + Array.ForEach(m_parts.GetArray(), p => p.Inventory.StopScriptInstances()); + } + /// /// Add an inventory item from a user's inventory to a prim in this scene object. /// diff --git a/OpenSim/Region/Framework/Scenes/SceneObjectPartInventory.cs b/OpenSim/Region/Framework/Scenes/SceneObjectPartInventory.cs index 866311a298..cf2ed1a802 100644 --- a/OpenSim/Region/Framework/Scenes/SceneObjectPartInventory.cs +++ b/OpenSim/Region/Framework/Scenes/SceneObjectPartInventory.cs @@ -280,7 +280,7 @@ namespace OpenSim.Region.Framework.Scenes } /// - /// Stop all the scripts in this prim. + /// Stop and remove all the scripts in this prim. /// /// /// Should be true if these scripts are being removed because the scene @@ -293,6 +293,14 @@ namespace OpenSim.Region.Framework.Scenes RemoveScriptInstance(item.ItemID, sceneObjectBeingDeleted); } + /// + /// Stop all the scripts in this prim. + /// + public void StopScriptInstances() + { + GetInventoryItems(InventoryType.LSL).ForEach(i => StopScriptInstance(i)); + } + /// /// Start a script which is in this prim's inventory. /// @@ -443,7 +451,7 @@ namespace OpenSim.Region.Framework.Scenes } /// - /// Stop a script which is in this prim's inventory. + /// Stop and remove a script which is in this prim's inventory. /// /// /// @@ -470,7 +478,7 @@ namespace OpenSim.Region.Framework.Scenes } else { - m_log.ErrorFormat( + m_log.WarnFormat( "[PRIM INVENTORY]: " + "Couldn't stop script with ID {0} since it couldn't be found for prim {1}, {2} at {3} in {4}", itemId, m_part.Name, m_part.UUID, @@ -478,6 +486,51 @@ namespace OpenSim.Region.Framework.Scenes } } + /// + /// Stop a script which is in this prim's inventory. + /// + /// + /// + /// Should be true if this script is being removed because the scene + /// object is being deleted. This will prevent spurious updates to the client. + /// + public void StopScriptInstance(UUID itemId) + { + TaskInventoryItem scriptItem; + + lock (m_items) + m_items.TryGetValue(itemId, out scriptItem); + + if (scriptItem != null) + { + StopScriptInstance(scriptItem); + } + else + { + m_log.WarnFormat( + "[PRIM INVENTORY]: " + + "Couldn't stop script with ID {0} since it couldn't be found for prim {1}, {2} at {3} in {4}", + itemId, m_part.Name, m_part.UUID, + m_part.AbsolutePosition, m_part.ParentGroup.Scene.RegionInfo.RegionName); + } + } + + /// + /// Stop a script which is in this prim's inventory. + /// + /// + /// + /// Should be true if this script is being removed because the scene + /// object is being deleted. This will prevent spurious updates to the client. + /// + public void StopScriptInstance(TaskInventoryItem item) + { + m_part.ParentGroup.Scene.EventManager.TriggerStopScript(m_part.LocalId, item.ItemID); + + // At the moment, even stopped scripts are counted as active, which is probably wrong. +// m_part.ParentGroup.AddActiveScriptCount(-1); + } + /// /// Check if the inventory holds an item with a given name. /// From 58869e5aa09a292dc2159c73bada2c487151dda0 Mon Sep 17 00:00:00 2001 From: "Justin Clark-Casey (justincc)" Date: Tue, 10 Jul 2012 23:03:52 +0100 Subject: [PATCH 3/5] Fix recent SOP.GetSittingAvatars() to return null if there are no sitting avatars rather than throwing an exception. Extends sitting avatar regression tests to test new sitters information --- .../Framework/Scenes/SceneObjectPart.cs | 14 +++++++++-- .../Scenes/Tests/ScenePresenceSitTests.cs | 24 +++++++++++++++---- prebuild.xml | 1 + 3 files changed, 33 insertions(+), 6 deletions(-) diff --git a/OpenSim/Region/Framework/Scenes/SceneObjectPart.cs b/OpenSim/Region/Framework/Scenes/SceneObjectPart.cs index 6518b84e49..6677daeb30 100644 --- a/OpenSim/Region/Framework/Scenes/SceneObjectPart.cs +++ b/OpenSim/Region/Framework/Scenes/SceneObjectPart.cs @@ -4556,10 +4556,20 @@ namespace OpenSim.Region.Framework.Scenes /// Get a copy of the list of sitting avatars. /// /// This applies to all sitting avatars whether there is a sit target set or not. - /// + /// A hashset of the sitting avatars. Returns null if there are no sitting avatars. public HashSet GetSittingAvatars() { - return new HashSet(m_sittingAvatars); + HashSet sittingAvatars = m_sittingAvatars; + + if (sittingAvatars == null) + { + return null; + } + else + { + lock (sittingAvatars) + return new HashSet(sittingAvatars); + } } /// diff --git a/OpenSim/Region/Framework/Scenes/Tests/ScenePresenceSitTests.cs b/OpenSim/Region/Framework/Scenes/Tests/ScenePresenceSitTests.cs index ed39be1fec..493ab70883 100644 --- a/OpenSim/Region/Framework/Scenes/Tests/ScenePresenceSitTests.cs +++ b/OpenSim/Region/Framework/Scenes/Tests/ScenePresenceSitTests.cs @@ -26,6 +26,7 @@ */ using System; +using System.Collections.Generic; using System.Reflection; using Nini.Config; using NUnit.Framework; @@ -69,6 +70,8 @@ namespace OpenSim.Region.Framework.Scenes.Tests m_sp.HandleAgentRequestSit(m_sp.ControllingClient, m_sp.UUID, part.UUID, Vector3.Zero); Assert.That(part.SitTargetAvatar, Is.EqualTo(UUID.Zero)); + Assert.That(part.GetSittingAvatarsCount(), Is.EqualTo(0)); + Assert.That(part.GetSittingAvatars(), Is.Null); Assert.That(m_sp.ParentID, Is.EqualTo(0)); } @@ -86,7 +89,13 @@ namespace OpenSim.Region.Framework.Scenes.Tests m_sp.HandleAgentRequestSit(m_sp.ControllingClient, m_sp.UUID, part.UUID, Vector3.Zero); + Assert.That(m_sp.PhysicsActor, Is.Null); + Assert.That(part.SitTargetAvatar, Is.EqualTo(UUID.Zero)); + Assert.That(part.GetSittingAvatarsCount(), Is.EqualTo(1)); + HashSet sittingAvatars = part.GetSittingAvatars(); + Assert.That(sittingAvatars.Count, Is.EqualTo(1)); + Assert.That(sittingAvatars.Contains(m_sp.UUID)); Assert.That(m_sp.ParentID, Is.EqualTo(part.LocalId)); } @@ -104,10 +113,6 @@ namespace OpenSim.Region.Framework.Scenes.Tests m_sp.HandleAgentRequestSit(m_sp.ControllingClient, m_sp.UUID, part.UUID, Vector3.Zero); - Assert.That(part.SitTargetAvatar, Is.EqualTo(UUID.Zero)); - Assert.That(m_sp.ParentID, Is.EqualTo(part.LocalId)); - Assert.That(m_sp.PhysicsActor, Is.Null); - // FIXME: This is different for live avatars - z position is adjusted. This is half the height of the // default avatar. // Curiously, Vector3.ToString() will not display the last two places of the float. For example, @@ -119,6 +124,8 @@ namespace OpenSim.Region.Framework.Scenes.Tests m_sp.StandUp(); Assert.That(part.SitTargetAvatar, Is.EqualTo(UUID.Zero)); + Assert.That(part.GetSittingAvatarsCount(), Is.EqualTo(0)); + Assert.That(part.GetSittingAvatars(), Is.Null); Assert.That(m_sp.ParentID, Is.EqualTo(0)); Assert.That(m_sp.PhysicsActor, Is.Not.Null); } @@ -145,11 +152,20 @@ namespace OpenSim.Region.Framework.Scenes.Tests Is.EqualTo(part.AbsolutePosition + part.SitTargetPosition + ScenePresence.SIT_TARGET_ADJUSTMENT)); Assert.That(m_sp.PhysicsActor, Is.Null); + Assert.That(part.GetSittingAvatarsCount(), Is.EqualTo(1)); + HashSet sittingAvatars = part.GetSittingAvatars(); + Assert.That(sittingAvatars.Count, Is.EqualTo(1)); + Assert.That(sittingAvatars.Contains(m_sp.UUID)); + m_sp.StandUp(); Assert.That(part.SitTargetAvatar, Is.EqualTo(UUID.Zero)); Assert.That(m_sp.ParentID, Is.EqualTo(0)); Assert.That(m_sp.PhysicsActor, Is.Not.Null); + + Assert.That(part.SitTargetAvatar, Is.EqualTo(UUID.Zero)); + Assert.That(part.GetSittingAvatarsCount(), Is.EqualTo(0)); + Assert.That(part.GetSittingAvatars(), Is.Null); } [Test] diff --git a/prebuild.xml b/prebuild.xml index 45f58c7e7c..3419119ef6 100644 --- a/prebuild.xml +++ b/prebuild.xml @@ -3042,6 +3042,7 @@ ../../../bin/ + From 9f01c3d4087266a8aa9a372a87ff78cae971d0c5 Mon Sep 17 00:00:00 2001 From: "Justin Clark-Casey (justincc)" Date: Tue, 10 Jul 2012 23:04:44 +0100 Subject: [PATCH 4/5] Disable logging in regression test in OSSL_ApiAttachmentTests --- .../Region/ScriptEngine/Shared/Tests/OSSL_ApiAttachmentTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/OpenSim/Region/ScriptEngine/Shared/Tests/OSSL_ApiAttachmentTests.cs b/OpenSim/Region/ScriptEngine/Shared/Tests/OSSL_ApiAttachmentTests.cs index f5aa51843f..5ed1f3d2cf 100644 --- a/OpenSim/Region/ScriptEngine/Shared/Tests/OSSL_ApiAttachmentTests.cs +++ b/OpenSim/Region/ScriptEngine/Shared/Tests/OSSL_ApiAttachmentTests.cs @@ -179,7 +179,7 @@ namespace OpenSim.Region.ScriptEngine.Shared.Tests public void TestOsForceAttachToOtherAvatarFromInventory() { TestHelpers.InMethod(); - TestHelpers.EnableLogging(); +// TestHelpers.EnableLogging(); string taskInvObjItemName = "sphere"; UUID taskInvObjItemId = UUID.Parse("00000000-0000-0000-0000-100000000000"); From 506437b68449ab521f0ccf467ad642ad5e442d73 Mon Sep 17 00:00:00 2001 From: "Justin Clark-Casey (justincc)" Date: Tue, 10 Jul 2012 23:06:34 +0100 Subject: [PATCH 5/5] Remove log line accidentally left in SP.SendSitResponse() --- OpenSim/Region/Framework/Scenes/ScenePresence.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/OpenSim/Region/Framework/Scenes/ScenePresence.cs b/OpenSim/Region/Framework/Scenes/ScenePresence.cs index 51ca9bdeaa..c71bae9bc1 100644 --- a/OpenSim/Region/Framework/Scenes/ScenePresence.cs +++ b/OpenSim/Region/Framework/Scenes/ScenePresence.cs @@ -1895,7 +1895,7 @@ namespace OpenSim.Region.Framework.Scenes ) )); - m_log.DebugFormat("[SCENE PRESENCE]: {0} {1}", SitTargetisSet, SitTargetUnOccupied); +// m_log.DebugFormat("[SCENE PRESENCE]: {0} {1}", SitTargetisSet, SitTargetUnOccupied); if (PhysicsActor != null) m_sitAvatarHeight = PhysicsActor.Size.Z;