From 3ffd90496a366f2b64eb8daadf63a2b6ee05ad7a Mon Sep 17 00:00:00 2001 From: "Justin Clark-Casey (justincc)" Date: Thu, 16 Jan 2014 20:23:31 +0000 Subject: [PATCH] Prevent duplicate invocations or race dontision in SP.CompleteMovement() This can happen under poor network conditions if a viewer repeats the message send If this happens, physics actors can get orphaned, which unecessarily raises physics frame times --- .../Region/Framework/Scenes/ScenePresence.cs | 29 +++++- .../Scenes/Tests/ScenePresenceAgentTests.cs | 92 ++++++++----------- 2 files changed, 65 insertions(+), 56 deletions(-) diff --git a/OpenSim/Region/Framework/Scenes/ScenePresence.cs b/OpenSim/Region/Framework/Scenes/ScenePresence.cs index 49f70c4f28..63cca5697e 100644 --- a/OpenSim/Region/Framework/Scenes/ScenePresence.cs +++ b/OpenSim/Region/Framework/Scenes/ScenePresence.cs @@ -108,6 +108,16 @@ namespace OpenSim.Region.Framework.Scenes } } + /// + /// This exists to prevent race conditions between two CompleteMovement threads if the simulator is slow and + /// the viewer fires these in quick succession. + /// + /// + /// TODO: The child -> agent transition should be folded into LifecycleState and the CompleteMovement + /// regulation done there. + /// + private object m_completeMovementLock = new object(); + // private static readonly byte[] DEFAULT_TEXTURE = AvatarAppearance.GetDefaultTexture().GetBytes(); private static readonly Array DIR_CONTROL_FLAGS = Enum.GetValues(typeof(Dir_ControlFlags)); private static readonly Vector3 HEAD_ADJUSTMENT = new Vector3(0f, 0f, 0.3f); @@ -905,6 +915,7 @@ namespace OpenSim.Region.Framework.Scenes /// /// Turns a child agent into a root agent. /// + /// /// Child agents are logged into neighbouring sims largely to observe changes. Root agents exist when the /// avatar is actual in the sim. They can perform all actions. /// This change is made whenever an avatar enters a region, whether by crossing over from a neighbouring sim, @@ -912,8 +923,8 @@ namespace OpenSim.Region.Framework.Scenes /// /// This method is on the critical path for transferring an avatar from one region to another. Delay here /// delays that crossing. - /// - private void MakeRootAgent(Vector3 pos, bool isFlying) + /// + private bool MakeRootAgent(Vector3 pos, bool isFlying) { // m_log.InfoFormat( // "[SCENE]: Upgrading child to root agent for {0} in {1}", @@ -921,6 +932,10 @@ namespace OpenSim.Region.Framework.Scenes //m_log.DebugFormat("[SCENE]: known regions in {0}: {1}", Scene.RegionInfo.RegionName, KnownChildRegionHandles.Count); + lock (m_completeMovementLock) + if (!IsChildAgent) + return false; + IsChildAgent = false; // Must reset this here so that a teleport to a region next to an existing region does not keep the flag @@ -1070,6 +1085,7 @@ namespace OpenSim.Region.Framework.Scenes m_scene.EventManager.TriggerOnMakeRootAgent(this); + return true; } public int GetStateSource() @@ -1443,7 +1459,14 @@ namespace OpenSim.Region.Framework.Scenes } bool flying = ((m_AgentControlFlags & AgentManager.ControlFlags.AGENT_CONTROL_FLY) != 0); - MakeRootAgent(AbsolutePosition, flying); + if (!MakeRootAgent(AbsolutePosition, flying)) + { + m_log.DebugFormat( + "[SCENE PRESENCE]: Aborting CompleteMovement call for {0} in {1} as they are already root", + Name, Scene.Name); + + return; + } // Tell the client that we're totally ready ControllingClient.MoveAgentIntoRegion(m_scene.RegionInfo, AbsolutePosition, look); diff --git a/OpenSim/Region/Framework/Scenes/Tests/ScenePresenceAgentTests.cs b/OpenSim/Region/Framework/Scenes/Tests/ScenePresenceAgentTests.cs index d1aeaeef16..1ff1329d71 100644 --- a/OpenSim/Region/Framework/Scenes/Tests/ScenePresenceAgentTests.cs +++ b/OpenSim/Region/Framework/Scenes/Tests/ScenePresenceAgentTests.cs @@ -111,6 +111,45 @@ namespace OpenSim.Region.Framework.Scenes.Tests Assert.That(scene.GetScenePresences().Count, Is.EqualTo(1)); } + /// + /// Test that duplicate complete movement calls are ignored. + /// + /// + /// If duplicate calls are not ignored then there is a risk of race conditions or other unexpected effects. + /// + [Test] + public void TestDupeCompleteMovementCalls() + { + TestHelpers.InMethod(); +// TestHelpers.EnableLogging(); + + UUID spUuid = TestHelpers.ParseTail(0x1); + + TestScene scene = new SceneHelpers().SetupScene(); + + int makeRootAgentEvents = 0; + scene.EventManager.OnMakeRootAgent += spi => makeRootAgentEvents++; + + ScenePresence sp = SceneHelpers.AddScenePresence(scene, spUuid); + + Assert.That(makeRootAgentEvents, Is.EqualTo(1)); + + // Normally these would be invoked by a CompleteMovement message coming in to the UDP stack. But for + // convenience, here we will invoke it manually. + sp.CompleteMovement(sp.ControllingClient, true); + + Assert.That(makeRootAgentEvents, Is.EqualTo(1)); + + // Check rest of exepcted parameters. + Assert.That(scene.AuthenticateHandler.GetAgentCircuitData(spUuid), Is.Not.Null); + Assert.That(scene.AuthenticateHandler.GetAgentCircuits().Count, Is.EqualTo(1)); + + Assert.That(sp.IsChildAgent, Is.False); + Assert.That(sp.UUID, Is.EqualTo(spUuid)); + + Assert.That(scene.GetScenePresences().Count, Is.EqualTo(1)); + } + [Test] public void TestCreateDuplicateRootScenePresence() { @@ -249,58 +288,5 @@ namespace OpenSim.Region.Framework.Scenes.Tests // Assert.That(childPresence, Is.Not.Null); // Assert.That(childPresence.IsChildAgent, Is.True); } - -// /// -// /// Test adding a root agent to a scene. Doesn't yet actually complete crossing the agent into the scene. -// /// -// [Test] -// public void T010_TestAddRootAgent() -// { -// TestHelpers.InMethod(); -// -// string firstName = "testfirstname"; -// -// AgentCircuitData agent = new AgentCircuitData(); -// agent.AgentID = agent1; -// agent.firstname = firstName; -// agent.lastname = "testlastname"; -// agent.SessionID = UUID.Random(); -// agent.SecureSessionID = UUID.Random(); -// agent.circuitcode = 123; -// agent.BaseFolder = UUID.Zero; -// agent.InventoryFolder = UUID.Zero; -// agent.startpos = Vector3.Zero; -// agent.CapsPath = GetRandomCapsObjectPath(); -// agent.ChildrenCapSeeds = new Dictionary(); -// agent.child = true; -// -// scene.PresenceService.LoginAgent(agent.AgentID.ToString(), agent.SessionID, agent.SecureSessionID); -// -// string reason; -// scene.NewUserConnection(agent, (uint)TeleportFlags.ViaLogin, out reason); -// testclient = new TestClient(agent, scene); -// scene.AddNewAgent(testclient); -// -// ScenePresence presence = scene.GetScenePresence(agent1); -// -// Assert.That(presence, Is.Not.Null, "presence is null"); -// Assert.That(presence.Firstname, Is.EqualTo(firstName), "First name not same"); -// acd1 = agent; -// } -// -// /// -// /// Test removing an uncrossed root agent from a scene. -// /// -// [Test] -// public void T011_TestRemoveRootAgent() -// { -// TestHelpers.InMethod(); -// -// scene.RemoveClient(agent1); -// -// ScenePresence presence = scene.GetScenePresence(agent1); -// -// Assert.That(presence, Is.Null, "presence is not null"); -// } } } \ No newline at end of file