From 47e2922a4099a419c82b49543a3ba551fa76adc4 Mon Sep 17 00:00:00 2001 From: "Justin Clark-Casey (justincc)" Date: Thu, 19 Jul 2012 22:32:27 +0100 Subject: [PATCH] Prevent race conditions between two threads that call LLClientView.Close() simultaneously (e.g. ack timeout and an attempt to reconnect) --- .../ClientStack/Linden/UDP/LLClientView.cs | 58 ++++++++++++++----- .../ClientStack/Linden/UDP/LLUDPServer.cs | 29 +++++----- OpenSim/Region/Framework/Scenes/Scene.cs | 6 +- 3 files changed, 60 insertions(+), 33 deletions(-) diff --git a/OpenSim/Region/ClientStack/Linden/UDP/LLClientView.cs b/OpenSim/Region/ClientStack/Linden/UDP/LLClientView.cs index ea53a46a53..1addf36f3b 100644 --- a/OpenSim/Region/ClientStack/Linden/UDP/LLClientView.cs +++ b/OpenSim/Region/ClientStack/Linden/UDP/LLClientView.cs @@ -348,8 +348,6 @@ namespace OpenSim.Region.ClientStack.LindenUDP private int m_animationSequenceNumber = 1; private bool m_SendLogoutPacketWhenClosing = true; private AgentUpdateArgs lastarg; - private bool m_IsActive = true; - private bool m_IsLoggingOut = false; protected Dictionary m_packetHandlers = new Dictionary(); protected Dictionary m_genericPacketHandlers = new Dictionary(); //PauPaw:Local Generic Message handlers @@ -413,16 +411,19 @@ namespace OpenSim.Region.ClientStack.LindenUDP public uint CircuitCode { get { return m_circuitCode; } } public int MoneyBalance { get { return m_moneyBalance; } } public int NextAnimationSequenceNumber { get { return m_animationSequenceNumber++; } } - public bool IsActive - { - get { return m_IsActive; } - set { m_IsActive = value; } - } - public bool IsLoggingOut - { - get { return m_IsLoggingOut; } - set { m_IsLoggingOut = value; } - } + + /// + /// As well as it's function in IClientAPI, in LLClientView we are locking on this property in order to + /// prevent race conditions by different threads calling Close(). + /// + public bool IsActive { get; set; } + + /// + /// Used to synchronise threads when client is being closed. + /// + public Object CloseSyncLock { get; private set; } + + public bool IsLoggingOut { get; set; } public bool DisableFacelights { @@ -447,6 +448,8 @@ namespace OpenSim.Region.ClientStack.LindenUDP { // DebugPacketLevel = 1; + CloseSyncLock = new Object(); + RegisterInterface(this); RegisterInterface(this); RegisterInterface(this); @@ -482,17 +485,40 @@ namespace OpenSim.Region.ClientStack.LindenUDP m_prioritizer = new Prioritizer(m_scene); RegisterLocalPacketHandlers(); + + IsActive = true; } #region Client Methods /// - /// Shut down the client view + /// Close down the client view /// public void Close() { - IsActive = false; + // We lock here to prevent race conditions between two threads calling close simultaneously (e.g. + // a simultaneous relog just as a client is being closed out due to no packet ack from the old connection. + lock (CloseSyncLock) + { + if (!IsActive) + return; + IsActive = false; + CloseWithoutChecks(); + } + } + + /// + /// Closes down the client view without first checking whether it is active. + /// + /// + /// This exists because LLUDPServer has to set IsActive = false in earlier synchronous code before calling + /// CloseWithoutIsActiveCheck asynchronously. + /// + /// Callers must lock ClosingSyncLock before calling. + /// + public void CloseWithoutChecks() + { m_log.DebugFormat( "[CLIENT]: Close has been called for {0} attached to scene {1}", Name, m_scene.RegionInfo.RegionName); @@ -3570,7 +3596,9 @@ namespace OpenSim.Region.ClientStack.LindenUDP public void SendCoarseLocationUpdate(List users, List CoarseLocations) { - if (!IsActive) return; // We don't need to update inactive clients. + // We don't need to update inactive clients. + if (!IsActive) + return; CoarseLocationUpdatePacket loc = (CoarseLocationUpdatePacket)PacketPool.Instance.GetPacket(PacketType.CoarseLocationUpdate); loc.Header.Reliable = false; diff --git a/OpenSim/Region/ClientStack/Linden/UDP/LLUDPServer.cs b/OpenSim/Region/ClientStack/Linden/UDP/LLUDPServer.cs index 097f1098eb..746eb90996 100644 --- a/OpenSim/Region/ClientStack/Linden/UDP/LLUDPServer.cs +++ b/OpenSim/Region/ClientStack/Linden/UDP/LLUDPServer.cs @@ -1123,22 +1123,21 @@ namespace OpenSim.Region.ClientStack.LindenUDP /// regular client pings. /// /// - private void DeactivateClientDueToTimeout(IClientAPI client) + private void DeactivateClientDueToTimeout(LLClientView client) { - // We must set IsActive synchronously so that we can stop the packet loop reinvoking this method, even - // though it's set later on by LLClientView.Close() - client.IsActive = false; - - m_log.WarnFormat( - "[LLUDPSERVER]: Ack timeout, disconnecting {0} agent for {1} in {2}", - client.SceneAgent.IsChildAgent ? "child" : "root", client.Name, m_scene.RegionInfo.RegionName); - - StatsManager.SimExtraStats.AddAbnormalClientThreadTermination(); - - if (!client.SceneAgent.IsChildAgent) - client.Kick("Simulator logged you out due to connection timeout"); - - client.Close(); + lock (client.CloseSyncLock) + { + m_log.WarnFormat( + "[LLUDPSERVER]: Ack timeout, disconnecting {0} agent for {1} in {2}", + client.SceneAgent.IsChildAgent ? "child" : "root", client.Name, m_scene.RegionInfo.RegionName); + + StatsManager.SimExtraStats.AddAbnormalClientThreadTermination(); + + if (!client.SceneAgent.IsChildAgent) + client.Kick("Simulator logged you out due to connection timeout"); + + client.CloseWithoutChecks(); + } } private void IncomingPacketHandler() diff --git a/OpenSim/Region/Framework/Scenes/Scene.cs b/OpenSim/Region/Framework/Scenes/Scene.cs index c3288a3f58..4c9d6e2f89 100644 --- a/OpenSim/Region/Framework/Scenes/Scene.cs +++ b/OpenSim/Region/Framework/Scenes/Scene.cs @@ -3420,8 +3420,8 @@ namespace OpenSim.Region.Framework.Scenes // We have a zombie from a crashed session. // Or the same user is trying to be root twice here, won't work. // Kill it. - m_log.DebugFormat( - "[SCENE]: Zombie scene presence detected for {0} {1} in {2}", + m_log.WarnFormat( + "[SCENE]: Existing root scene presence detected for {0} {1} in {2} when connecting. Removing existing presence.", sp.Name, sp.UUID, RegionInfo.RegionName); sp.ControllingClient.Close(); @@ -4377,7 +4377,7 @@ namespace OpenSim.Region.Framework.Scenes /// /// /// This method will return both root and child scene presences. - /// + /// /// Consider using ForEachScenePresence() or ForEachRootScenePresence() if possible since these will not /// involving creating a new List object. ///