From 859646ef5cf016eecfefb53b2b388fe880c39a3a Mon Sep 17 00:00:00 2001 From: Justin Clark-Casey (justincc) Date: Tue, 17 Apr 2012 23:54:51 +0100 Subject: minor: Add some method doc. Add warnings since calling SOG link/delink methods directly rather than through Scene may allow race conditions. --- OpenSim/Region/Framework/Scenes/SceneGraph.cs | 3 +++ 1 file changed, 3 insertions(+) (limited to 'OpenSim/Region/Framework/Scenes/SceneGraph.cs') diff --git a/OpenSim/Region/Framework/Scenes/SceneGraph.cs b/OpenSim/Region/Framework/Scenes/SceneGraph.cs index 0098add..67eb0fe 100644 --- a/OpenSim/Region/Framework/Scenes/SceneGraph.cs +++ b/OpenSim/Region/Framework/Scenes/SceneGraph.cs @@ -91,6 +91,9 @@ namespace OpenSim.Region.Framework.Scenes /// protected internal Dictionary SceneObjectGroupsByLocalPartID = new Dictionary(); + /// + /// Lock to prevent object group update, linking and delinking operations from running concurrently. + /// private Object m_updateLock = new Object(); #endregion -- cgit v1.1 From abbd050a13e2b4f174c37979b46cf83f6b2f62dc Mon Sep 17 00:00:00 2001 From: Justin Clark-Casey (justincc) Date: Tue, 8 May 2012 21:31:35 +0100 Subject: Perform SceneGraph.DuplicateObject() under existing m_updateLock already used for link and delinking, in order to avoid race conditions. DuplicateObject() relies on source object having correct link numbers for the duration of the dupe. Both link and delink can change link numbers such that they are not consistent for short periods of time. --- OpenSim/Region/Framework/Scenes/SceneGraph.cs | 164 ++++++++++++++------------ 1 file changed, 89 insertions(+), 75 deletions(-) (limited to 'OpenSim/Region/Framework/Scenes/SceneGraph.cs') diff --git a/OpenSim/Region/Framework/Scenes/SceneGraph.cs b/OpenSim/Region/Framework/Scenes/SceneGraph.cs index 67eb0fe..4815922 100644 --- a/OpenSim/Region/Framework/Scenes/SceneGraph.cs +++ b/OpenSim/Region/Framework/Scenes/SceneGraph.cs @@ -92,8 +92,12 @@ namespace OpenSim.Region.Framework.Scenes protected internal Dictionary SceneObjectGroupsByLocalPartID = new Dictionary(); /// - /// Lock to prevent object group update, linking and delinking operations from running concurrently. + /// Lock to prevent object group update, linking, delinking and duplication operations from running concurrently. /// + /// + /// These operations rely on the parts composition of the object. If allowed to run concurrently then race + /// conditions can occur. + /// private Object m_updateLock = new Object(); #endregion @@ -1844,96 +1848,106 @@ namespace OpenSim.Region.Framework.Scenes /// /// /// - public SceneObjectGroup DuplicateObject(uint originalPrimID, Vector3 offset, uint flags, UUID AgentID, UUID GroupID, Quaternion rot) + /// null if duplication fails, otherwise the duplicated object + public SceneObjectGroup DuplicateObject( + uint originalPrimID, Vector3 offset, uint flags, UUID AgentID, UUID GroupID, Quaternion rot) { -// m_log.DebugFormat( -// "[SCENE]: Duplication of object {0} at offset {1} requested by agent {2}", -// originalPrimID, offset, AgentID); - - SceneObjectGroup original = GetGroupByPrim(originalPrimID); - if (original != null) + Monitor.Enter(m_updateLock); + + try { - if (m_parentScene.Permissions.CanDuplicateObject( - original.PrimCount, original.UUID, AgentID, original.AbsolutePosition)) + // m_log.DebugFormat( + // "[SCENE]: Duplication of object {0} at offset {1} requested by agent {2}", + // originalPrimID, offset, AgentID); + + SceneObjectGroup original = GetGroupByPrim(originalPrimID); + if (original == null) { - SceneObjectGroup copy = original.Copy(true); - copy.AbsolutePosition = copy.AbsolutePosition + offset; + m_log.WarnFormat( + "[SCENEGRAPH]: Attempt to duplicate nonexistant prim id {0} by {1}", originalPrimID, AgentID); - if (original.OwnerID != AgentID) - { - copy.SetOwnerId(AgentID); - copy.SetRootPartOwner(copy.RootPart, AgentID, GroupID); + return null; + } - SceneObjectPart[] partList = copy.Parts; + if (!m_parentScene.Permissions.CanDuplicateObject( + original.PrimCount, original.UUID, AgentID, original.AbsolutePosition)) + return null; - if (m_parentScene.Permissions.PropagatePermissions()) - { - foreach (SceneObjectPart child in partList) - { - child.Inventory.ChangeInventoryOwner(AgentID); - child.TriggerScriptChangedEvent(Changed.OWNER); - child.ApplyNextOwnerPermissions(); - } - } + SceneObjectGroup copy = original.Copy(true); + copy.AbsolutePosition = copy.AbsolutePosition + offset; - copy.RootPart.ObjectSaleType = 0; - copy.RootPart.SalePrice = 10; - } + if (original.OwnerID != AgentID) + { + copy.SetOwnerId(AgentID); + copy.SetRootPartOwner(copy.RootPart, AgentID, GroupID); - // FIXME: This section needs to be refactored so that it just calls AddSceneObject() - Entities.Add(copy); - - lock (SceneObjectGroupsByFullID) - SceneObjectGroupsByFullID[copy.UUID] = copy; - - SceneObjectPart[] children = copy.Parts; - - lock (SceneObjectGroupsByFullPartID) - { - SceneObjectGroupsByFullPartID[copy.UUID] = copy; - foreach (SceneObjectPart part in children) - SceneObjectGroupsByFullPartID[part.UUID] = copy; - } - - lock (SceneObjectGroupsByLocalPartID) - { - SceneObjectGroupsByLocalPartID[copy.LocalId] = copy; - foreach (SceneObjectPart part in children) - SceneObjectGroupsByLocalPartID[part.LocalId] = copy; - } - // PROBABLE END OF FIXME - - // Since we copy from a source group that is in selected - // state, but the copy is shown deselected in the viewer, - // We need to clear the selection flag here, else that - // prim never gets persisted at all. The client doesn't - // think it's selected, so it will never send a deselect... - copy.IsSelected = false; - - m_numPrim += copy.Parts.Length; - - if (rot != Quaternion.Identity) + SceneObjectPart[] partList = copy.Parts; + + if (m_parentScene.Permissions.PropagatePermissions()) { - copy.UpdateGroupRotationR(rot); + foreach (SceneObjectPart child in partList) + { + child.Inventory.ChangeInventoryOwner(AgentID); + child.TriggerScriptChangedEvent(Changed.OWNER); + child.ApplyNextOwnerPermissions(); + } } - copy.CreateScriptInstances(0, false, m_parentScene.DefaultScriptEngine, 1); - copy.HasGroupChanged = true; - copy.ScheduleGroupForFullUpdate(); - copy.ResumeScripts(); - - // required for physics to update it's position - copy.AbsolutePosition = copy.AbsolutePosition; + copy.RootPart.ObjectSaleType = 0; + copy.RootPart.SalePrice = 10; + } - return copy; + // FIXME: This section needs to be refactored so that it just calls AddSceneObject() + Entities.Add(copy); + + lock (SceneObjectGroupsByFullID) + SceneObjectGroupsByFullID[copy.UUID] = copy; + + SceneObjectPart[] children = copy.Parts; + + lock (SceneObjectGroupsByFullPartID) + { + SceneObjectGroupsByFullPartID[copy.UUID] = copy; + foreach (SceneObjectPart part in children) + SceneObjectGroupsByFullPartID[part.UUID] = copy; + } + + lock (SceneObjectGroupsByLocalPartID) + { + SceneObjectGroupsByLocalPartID[copy.LocalId] = copy; + foreach (SceneObjectPart part in children) + SceneObjectGroupsByLocalPartID[part.LocalId] = copy; + } + // PROBABLE END OF FIXME + + // Since we copy from a source group that is in selected + // state, but the copy is shown deselected in the viewer, + // We need to clear the selection flag here, else that + // prim never gets persisted at all. The client doesn't + // think it's selected, so it will never send a deselect... + copy.IsSelected = false; + + m_numPrim += copy.Parts.Length; + + if (rot != Quaternion.Identity) + { + copy.UpdateGroupRotationR(rot); } + + copy.CreateScriptInstances(0, false, m_parentScene.DefaultScriptEngine, 1); + copy.HasGroupChanged = true; + copy.ScheduleGroupForFullUpdate(); + copy.ResumeScripts(); + + // required for physics to update it's position + copy.AbsolutePosition = copy.AbsolutePosition; + + return copy; } - else + finally { - m_log.WarnFormat("[SCENE]: Attempted to duplicate nonexistant prim id {0}", GroupID); + Monitor.Exit(m_updateLock); } - - return null; } /// -- cgit v1.1 From 4d34763f8c90dfe87a8a5930bf05fe36b86d15df Mon Sep 17 00:00:00 2001 From: Justin Clark-Casey (justincc) Date: Thu, 17 May 2012 23:33:26 +0100 Subject: Check agent limit against root agent count rather than both root and child agents From sl docs such as http://community.secondlife.com/t5/English-Knowledge-Base/Managing-Private-Regions/ta-p/700115 agent should apply to avatars only. This makes sense from a user perspective, and also from a code perspective since child agents with no physics or actions take up a fraction of root agent resources. As such, the check is now only performed in Scene.QueryAccess() - cross and teleport check this before allowing an agent to translocate. This also removes an off-by-one error that could occur in certain circumstances on teleport when a new child agent was double counted when a pre-teleport agent update was performed. This does not affect an existing bug where limits or other QueryAccess() checks are not applied to avatars logging directly into a region. --- OpenSim/Region/Framework/Scenes/SceneGraph.cs | 5 ----- 1 file changed, 5 deletions(-) (limited to 'OpenSim/Region/Framework/Scenes/SceneGraph.cs') diff --git a/OpenSim/Region/Framework/Scenes/SceneGraph.cs b/OpenSim/Region/Framework/Scenes/SceneGraph.cs index 4815922..ddf1550 100644 --- a/OpenSim/Region/Framework/Scenes/SceneGraph.cs +++ b/OpenSim/Region/Framework/Scenes/SceneGraph.cs @@ -779,11 +779,6 @@ namespace OpenSim.Region.Framework.Scenes return m_scenePresenceArray; } - public int GetNumberOfScenePresences() - { - return m_scenePresenceArray.Count; - } - /// /// Request a scene presence by UUID. Fast, indexed lookup. /// -- cgit v1.1 From d547bcf8d15bdcb8604cf170dc8ec1e14ad4cdc0 Mon Sep 17 00:00:00 2001 From: Justin Clark-Casey (justincc) Date: Fri, 8 Jun 2012 00:40:38 +0100 Subject: Remove duplicate update of user count in Scene.IncomingCloseAgent() This is already done in Scene.RemoveClient() which IncomingCloseAgent() always ends up calling. --- OpenSim/Region/Framework/Scenes/SceneGraph.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'OpenSim/Region/Framework/Scenes/SceneGraph.cs') diff --git a/OpenSim/Region/Framework/Scenes/SceneGraph.cs b/OpenSim/Region/Framework/Scenes/SceneGraph.cs index ddf1550..82a4f64 100644 --- a/OpenSim/Region/Framework/Scenes/SceneGraph.cs +++ b/OpenSim/Region/Framework/Scenes/SceneGraph.cs @@ -703,10 +703,10 @@ namespace OpenSim.Region.Framework.Scenes public int GetChildAgentCount() { // some network situations come in where child agents get closed twice. - if (m_numChildAgents < 0) - { - m_numChildAgents = 0; - } +// if (m_numChildAgents < 0) +// { +// m_numChildAgents = 0; +// } return m_numChildAgents; } -- cgit v1.1 From 5c162ccd57639f0c711d9940ecdd3e2804d26304 Mon Sep 17 00:00:00 2001 From: Justin Clark-Casey (justincc) Date: Fri, 8 Jun 2012 00:59:39 +0100 Subject: Go back to calling IncomingCloseAgent() in the "kick user" command for consistency instead of IClientAPI.Close() directly. This no longer double counts child agent removals --- OpenSim/Region/Framework/Scenes/SceneGraph.cs | 6 ------ 1 file changed, 6 deletions(-) (limited to 'OpenSim/Region/Framework/Scenes/SceneGraph.cs') diff --git a/OpenSim/Region/Framework/Scenes/SceneGraph.cs b/OpenSim/Region/Framework/Scenes/SceneGraph.cs index 82a4f64..a59758f 100644 --- a/OpenSim/Region/Framework/Scenes/SceneGraph.cs +++ b/OpenSim/Region/Framework/Scenes/SceneGraph.cs @@ -702,12 +702,6 @@ namespace OpenSim.Region.Framework.Scenes public int GetChildAgentCount() { - // some network situations come in where child agents get closed twice. -// if (m_numChildAgents < 0) -// { -// m_numChildAgents = 0; -// } - return m_numChildAgents; } -- cgit v1.1 From 056c9a59b2fee1e459915bd1ca908107c7c9695d Mon Sep 17 00:00:00 2001 From: Justin Clark-Casey (justincc) Date: Fri, 6 Jul 2012 23:07:50 +0100 Subject: Add assert to attachment regression tests to check that number of objects in the scene graph --- OpenSim/Region/Framework/Scenes/SceneGraph.cs | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) (limited to 'OpenSim/Region/Framework/Scenes/SceneGraph.cs') diff --git a/OpenSim/Region/Framework/Scenes/SceneGraph.cs b/OpenSim/Region/Framework/Scenes/SceneGraph.cs index a59758f..bc9a585 100644 --- a/OpenSim/Region/Framework/Scenes/SceneGraph.cs +++ b/OpenSim/Region/Framework/Scenes/SceneGraph.cs @@ -958,6 +958,18 @@ namespace OpenSim.Region.Framework.Scenes } /// + /// Get all the scene object groups. + /// + /// + /// The scene object groups. If the scene is empty then an empty list is returned. + /// + protected internal List GetSceneObjectGroups() + { + lock (SceneObjectGroupsByFullID) + return new List(SceneObjectGroupsByFullID.Values); + } + + /// /// Get a group in the scene /// /// UUID of the group @@ -1100,11 +1112,7 @@ namespace OpenSim.Region.Framework.Scenes /// protected internal void ForEachSOG(Action action) { - List objlist; - lock (SceneObjectGroupsByFullID) - objlist = new List(SceneObjectGroupsByFullID.Values); - - foreach (SceneObjectGroup obj in objlist) + foreach (SceneObjectGroup obj in GetSceneObjectGroups()) { try { -- cgit v1.1 From 7ff4eec79cf16886439604a23126abd195ae8b2e Mon Sep 17 00:00:00 2001 From: Justin Clark-Casey (justincc) Date: Sat, 7 Jul 2012 00:02:45 +0100 Subject: Remove redundant SetScene() function in Scene.AddSceneObject() This is always done later on in SceneGraph.AddSceneObject() if the call hasn't failed due to sanity checks. There's no other purpose for this method to exist and it's dangerous/pointless to call in other conditions. --- OpenSim/Region/Framework/Scenes/SceneGraph.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'OpenSim/Region/Framework/Scenes/SceneGraph.cs') diff --git a/OpenSim/Region/Framework/Scenes/SceneGraph.cs b/OpenSim/Region/Framework/Scenes/SceneGraph.cs index bc9a585..2be5364 100644 --- a/OpenSim/Region/Framework/Scenes/SceneGraph.cs +++ b/OpenSim/Region/Framework/Scenes/SceneGraph.cs @@ -355,9 +355,9 @@ namespace OpenSim.Region.Framework.Scenes if (Entities.ContainsKey(sceneObject.UUID)) { -// m_log.DebugFormat( -// "[SCENEGRAPH]: Scene graph for {0} already contains object {1} in AddSceneObject()", -// m_parentScene.RegionInfo.RegionName, sceneObject.UUID); + m_log.DebugFormat( + "[SCENEGRAPH]: Scene graph for {0} already contains object {1} in AddSceneObject()", + m_parentScene.RegionInfo.RegionName, sceneObject.UUID); return false; } -- cgit v1.1