From ba80f137b58cfacf46fadb3ec8b63af6896c5b43 Mon Sep 17 00:00:00 2001 From: Justin Clark-Casey (justincc) Date: Thu, 19 Jul 2012 22:32:27 +0100 Subject: Prevent race conditions between two threads that call LLClientView.Close() simultaneously (e.g. ack timeout and an attempt to reconnect) --- .../Region/ClientStack/Linden/UDP/LLClientView.cs | 58 ++++++++++++++++------ .../Region/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 73cdec3..ae5207b 100644 --- a/OpenSim/Region/ClientStack/Linden/UDP/LLClientView.cs +++ b/OpenSim/Region/ClientStack/Linden/UDP/LLClientView.cs @@ -347,8 +347,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 @@ -412,16 +410,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 { @@ -446,6 +447,8 @@ namespace OpenSim.Region.ClientStack.LindenUDP { // DebugPacketLevel = 1; + CloseSyncLock = new Object(); + RegisterInterface(this); RegisterInterface(this); RegisterInterface(this); @@ -478,17 +481,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); @@ -3567,7 +3593,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 097f109..746eb90 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 36452de..51a6820 100644 --- a/OpenSim/Region/Framework/Scenes/Scene.cs +++ b/OpenSim/Region/Framework/Scenes/Scene.cs @@ -3517,8 +3517,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(); @@ -4474,7 +4474,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. /// -- cgit v1.1