From 16f296f48932379a89c52f095eae199347f33407 Mon Sep 17 00:00:00 2001 From: Justin Clark-Casey (justincc) Date: Wed, 15 Sep 2010 21:41:42 +0100 Subject: Instead of locking SOG.Children when a group is being removed from the scene, iterate over an unlocked list instead Previously, deadlock was possible because deleting a group took a SOG.Children lock then an m_entityUpdates.SyncRoot lock in LLClientView At the same time, a thread starting from LLClientView.ProcessEntityUpdates() could take an m_entityUpdates.SyncRoot lock then later attempt to take a SOG.Children lock in PermissionsModule.GenerateClientFlags() and later on Taking a children list in SOG appears to be a better solution than changing PermissionsModule to not relook up the prim. Going the permission modules root would require that all downstream modules not take a SOG.Children lock either --- .../Region/Framework/Scenes/SceneObjectGroup.cs | 32 ++++++++++++---------- 1 file changed, 17 insertions(+), 15 deletions(-) (limited to 'OpenSim/Region') diff --git a/OpenSim/Region/Framework/Scenes/SceneObjectGroup.cs b/OpenSim/Region/Framework/Scenes/SceneObjectGroup.cs index dc6509d..99c2abf 100644 --- a/OpenSim/Region/Framework/Scenes/SceneObjectGroup.cs +++ b/OpenSim/Region/Framework/Scenes/SceneObjectGroup.cs @@ -29,6 +29,7 @@ using System; using System.Collections.Generic; using System.Drawing; using System.IO; +using System.Linq; using System.Threading; using System.Xml; using System.Xml.Serialization; @@ -1236,26 +1237,27 @@ namespace OpenSim.Region.Framework.Scenes /// If true then deletion is not broadcast to clients public void DeleteGroup(bool silent) { + List parts; + lock (m_parts) + parts = m_parts.Values.ToList(); + + foreach (SceneObjectPart part in parts) { - foreach (SceneObjectPart part in m_parts.Values) + Scene.ForEachScenePresence(delegate(ScenePresence avatar) { -// part.Inventory.RemoveScriptInstances(); - Scene.ForEachScenePresence(delegate(ScenePresence avatar) + if (avatar.ParentID == LocalId) { - if (avatar.ParentID == LocalId) - { - avatar.StandUp(); - } + avatar.StandUp(); + } - if (!silent) - { - part.UpdateFlag = 0; - if (part == m_rootPart) - avatar.ControllingClient.SendKillObject(m_regionHandle, part.LocalId); - } - }); - } + if (!silent) + { + part.UpdateFlag = 0; + if (part == m_rootPart) + avatar.ControllingClient.SendKillObject(m_regionHandle, part.LocalId); + } + }); } } -- cgit v1.1 From 39d27fc8795e642280cc255c5c1c4ff1c7a915ec Mon Sep 17 00:00:00 2001 From: Justin Clark-Casey (justincc) Date: Wed, 15 Sep 2010 22:29:58 +0100 Subject: rename SceneObjectGroup.DeleteGroup() to DeleteGroupFromScene() to improve code readability --- OpenSim/Region/Framework/Scenes/AsyncSceneObjectGroupDeleter.cs | 2 +- OpenSim/Region/Framework/Scenes/Scene.cs | 2 +- OpenSim/Region/Framework/Scenes/SceneObjectGroup.cs | 2 +- OpenSim/Region/OptionalModules/ContentManagementSystem/CMModel.cs | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) (limited to 'OpenSim/Region') diff --git a/OpenSim/Region/Framework/Scenes/AsyncSceneObjectGroupDeleter.cs b/OpenSim/Region/Framework/Scenes/AsyncSceneObjectGroupDeleter.cs index 916148b..64567db 100644 --- a/OpenSim/Region/Framework/Scenes/AsyncSceneObjectGroupDeleter.cs +++ b/OpenSim/Region/Framework/Scenes/AsyncSceneObjectGroupDeleter.cs @@ -105,7 +105,7 @@ namespace OpenSim.Region.Framework.Scenes if (permissionToDelete) { foreach (SceneObjectGroup g in objectGroups) - g.DeleteGroup(false); + g.DeleteGroupFromScene(false); } } diff --git a/OpenSim/Region/Framework/Scenes/Scene.cs b/OpenSim/Region/Framework/Scenes/Scene.cs index ef97dfc..8a90bc8 100644 --- a/OpenSim/Region/Framework/Scenes/Scene.cs +++ b/OpenSim/Region/Framework/Scenes/Scene.cs @@ -2120,7 +2120,7 @@ namespace OpenSim.Region.Framework.Scenes EventManager.TriggerParcelPrimCountTainted(); } - group.DeleteGroup(silent); + group.DeleteGroupFromScene(silent); // m_log.DebugFormat("[SCENE]: Exit DeleteSceneObject() for {0} {1}", group.Name, group.UUID); } diff --git a/OpenSim/Region/Framework/Scenes/SceneObjectGroup.cs b/OpenSim/Region/Framework/Scenes/SceneObjectGroup.cs index 99c2abf..f9a8d41 100644 --- a/OpenSim/Region/Framework/Scenes/SceneObjectGroup.cs +++ b/OpenSim/Region/Framework/Scenes/SceneObjectGroup.cs @@ -1235,7 +1235,7 @@ namespace OpenSim.Region.Framework.Scenes /// must be handled by the caller. /// /// If true then deletion is not broadcast to clients - public void DeleteGroup(bool silent) + public void DeleteGroupFromScene(bool silent) { List parts; diff --git a/OpenSim/Region/OptionalModules/ContentManagementSystem/CMModel.cs b/OpenSim/Region/OptionalModules/ContentManagementSystem/CMModel.cs index fd59138..0b02a9f 100644 --- a/OpenSim/Region/OptionalModules/ContentManagementSystem/CMModel.cs +++ b/OpenSim/Region/OptionalModules/ContentManagementSystem/CMModel.cs @@ -263,7 +263,7 @@ namespace OpenSim.Region.OptionalModules.ContentManagement scene.PhysicsScene.RemovePrim(((SceneObjectGroup)scene.Entities[uuid]).RootPart.PhysActor); scene.SendKillObject(scene.Entities[uuid].LocalId); scene.SceneGraph.DeleteSceneObject(uuid, false); - ((SceneObjectGroup)scene.Entities[uuid]).DeleteGroup(false); + ((SceneObjectGroup)scene.Entities[uuid]).DeleteGroupFromScene(false); } catch(Exception e) { -- cgit v1.1 From 7383173d3d33b9aeb5e097b2bc55959ede822e94 Mon Sep 17 00:00:00 2001 From: Justin Clark-Casey (justincc) Date: Wed, 15 Sep 2010 23:06:38 +0100 Subject: extend m_entityUpdates.SyncRoot lock in LLClientView.ProcessEntityUpdates() to reduce scope for kill/update race conditions This is necessary because it was still possible for an entity update packet to be constructed, the thread to pause, a kill to be sent on another thread, and then the original thread to resume and send the update This would result in an update being received after a kill, which results in undeletable ghost objects until the viewer is relogged Extending the lock looks okay since its only taken by kill, update and reprioritize, and both kill and update do not take further locks However, evidence suggests that there is still a kill/update race somewhere --- .../Region/ClientStack/LindenUDP/LLClientView.cs | 98 +++++++++++----------- 1 file changed, 49 insertions(+), 49 deletions(-) (limited to 'OpenSim/Region') diff --git a/OpenSim/Region/ClientStack/LindenUDP/LLClientView.cs b/OpenSim/Region/ClientStack/LindenUDP/LLClientView.cs index 0aa670a..2163c12 100644 --- a/OpenSim/Region/ClientStack/LindenUDP/LLClientView.cs +++ b/OpenSim/Region/ClientStack/LindenUDP/LLClientView.cs @@ -3678,56 +3678,56 @@ namespace OpenSim.Region.ClientStack.LindenUDP #endregion Block Construction } - } - - #region Packet Sending - - const float TIME_DILATION = 1.0f; - ushort timeDilation = Utils.FloatToUInt16(TIME_DILATION, 0.0f, 1.0f); - - if (objectUpdateBlocks.IsValueCreated) - { - List blocks = objectUpdateBlocks.Value; - - ObjectUpdatePacket packet = (ObjectUpdatePacket)PacketPool.Instance.GetPacket(PacketType.ObjectUpdate); - packet.RegionData.RegionHandle = m_scene.RegionInfo.RegionHandle; - packet.RegionData.TimeDilation = timeDilation; - packet.ObjectData = new ObjectUpdatePacket.ObjectDataBlock[blocks.Count]; - - for (int i = 0; i < blocks.Count; i++) - packet.ObjectData[i] = blocks[i]; - - OutPacket(packet, ThrottleOutPacketType.Task, true); - } - - if (compressedUpdateBlocks.IsValueCreated) - { - List blocks = compressedUpdateBlocks.Value; - - ObjectUpdateCompressedPacket packet = (ObjectUpdateCompressedPacket)PacketPool.Instance.GetPacket(PacketType.ObjectUpdateCompressed); - packet.RegionData.RegionHandle = m_scene.RegionInfo.RegionHandle; - packet.RegionData.TimeDilation = timeDilation; - packet.ObjectData = new ObjectUpdateCompressedPacket.ObjectDataBlock[blocks.Count]; - - for (int i = 0; i < blocks.Count; i++) - packet.ObjectData[i] = blocks[i]; - - OutPacket(packet, ThrottleOutPacketType.Task, true); - } - - if (terseUpdateBlocks.IsValueCreated) - { - List blocks = terseUpdateBlocks.Value; - - ImprovedTerseObjectUpdatePacket packet = new ImprovedTerseObjectUpdatePacket(); - packet.RegionData.RegionHandle = m_scene.RegionInfo.RegionHandle; - packet.RegionData.TimeDilation = timeDilation; - packet.ObjectData = new ImprovedTerseObjectUpdatePacket.ObjectDataBlock[blocks.Count]; - - for (int i = 0; i < blocks.Count; i++) - packet.ObjectData[i] = blocks[i]; - OutPacket(packet, ThrottleOutPacketType.Task, true); + #region Packet Sending + + const float TIME_DILATION = 1.0f; + ushort timeDilation = Utils.FloatToUInt16(TIME_DILATION, 0.0f, 1.0f); + + if (objectUpdateBlocks.IsValueCreated) + { + List blocks = objectUpdateBlocks.Value; + + ObjectUpdatePacket packet = (ObjectUpdatePacket)PacketPool.Instance.GetPacket(PacketType.ObjectUpdate); + packet.RegionData.RegionHandle = m_scene.RegionInfo.RegionHandle; + packet.RegionData.TimeDilation = timeDilation; + packet.ObjectData = new ObjectUpdatePacket.ObjectDataBlock[blocks.Count]; + + for (int i = 0; i < blocks.Count; i++) + packet.ObjectData[i] = blocks[i]; + + OutPacket(packet, ThrottleOutPacketType.Task, true); + } + + if (compressedUpdateBlocks.IsValueCreated) + { + List blocks = compressedUpdateBlocks.Value; + + ObjectUpdateCompressedPacket packet = (ObjectUpdateCompressedPacket)PacketPool.Instance.GetPacket(PacketType.ObjectUpdateCompressed); + packet.RegionData.RegionHandle = m_scene.RegionInfo.RegionHandle; + packet.RegionData.TimeDilation = timeDilation; + packet.ObjectData = new ObjectUpdateCompressedPacket.ObjectDataBlock[blocks.Count]; + + for (int i = 0; i < blocks.Count; i++) + packet.ObjectData[i] = blocks[i]; + + OutPacket(packet, ThrottleOutPacketType.Task, true); + } + + if (terseUpdateBlocks.IsValueCreated) + { + List blocks = terseUpdateBlocks.Value; + + ImprovedTerseObjectUpdatePacket packet = new ImprovedTerseObjectUpdatePacket(); + packet.RegionData.RegionHandle = m_scene.RegionInfo.RegionHandle; + packet.RegionData.TimeDilation = timeDilation; + packet.ObjectData = new ImprovedTerseObjectUpdatePacket.ObjectDataBlock[blocks.Count]; + + for (int i = 0; i < blocks.Count; i++) + packet.ObjectData[i] = blocks[i]; + + OutPacket(packet, ThrottleOutPacketType.Task, true); + } } #endregion Packet Sending -- cgit v1.1