From 921ad8704e08fccc994f7907e7cb1e9372e355b9 Mon Sep 17 00:00:00 2001 From: Justin Clark-Casey (justincc) Date: Fri, 31 May 2013 22:50:15 +0100 Subject: Lock areas of AsyncCommandManager where multiple threads could try to access/update the same static structures simultaneously. This is possible where there is more than one scene (multiple copies of the same script engine) and/or more than one script engine being used. These operations are not thread safe and could be leading to the exceptions/problems seen in http://opensimulator.org/mantis/view.php?id=6651 This also prevents a small race condition where more than one AsyncLSLCmdHandlerThread could be started. --- .../Api/Implementation/AsyncCommandManager.cs | 288 +++++++++++++-------- 1 file changed, 177 insertions(+), 111 deletions(-) diff --git a/OpenSim/Region/ScriptEngine/Shared/Api/Implementation/AsyncCommandManager.cs b/OpenSim/Region/ScriptEngine/Shared/Api/Implementation/AsyncCommandManager.cs index c71b571..72e8415 100644 --- a/OpenSim/Region/ScriptEngine/Shared/Api/Implementation/AsyncCommandManager.cs +++ b/OpenSim/Region/ScriptEngine/Shared/Api/Implementation/AsyncCommandManager.cs @@ -52,6 +52,15 @@ namespace OpenSim.Region.ScriptEngine.Shared.Api private static Thread cmdHandlerThread; private static int cmdHandlerThreadCycleSleepms; + /// + /// Lock for reading/writing static components of AsyncCommandManager. + /// + /// + /// This lock exists so that multiple threads from different engines and/or different copies of the same engine + /// are prevented from running non-thread safe code (e.g. read/write of lists) concurrently. + /// + private static object staticLock = new object(); + private static List m_Scenes = new List(); private static List m_ScriptEngines = new List(); @@ -74,37 +83,65 @@ namespace OpenSim.Region.ScriptEngine.Shared.Api public Dataserver DataserverPlugin { - get { return m_Dataserver[m_ScriptEngine]; } + get + { + lock (staticLock) + return m_Dataserver[m_ScriptEngine]; + } } public Timer TimerPlugin { - get { return m_Timer[m_ScriptEngine]; } + get + { + lock (staticLock) + return m_Timer[m_ScriptEngine]; + } } public HttpRequest HttpRequestPlugin { - get { return m_HttpRequest[m_ScriptEngine]; } + get + { + lock (staticLock) + return m_HttpRequest[m_ScriptEngine]; + } } public Listener ListenerPlugin { - get { return m_Listener[m_ScriptEngine]; } + get + { + lock (staticLock) + return m_Listener[m_ScriptEngine]; + } } public SensorRepeat SensorRepeatPlugin { - get { return m_SensorRepeat[m_ScriptEngine]; } + get + { + lock (staticLock) + return m_SensorRepeat[m_ScriptEngine]; + } } public XmlRequest XmlRequestPlugin { - get { return m_XmlRequest[m_ScriptEngine]; } + get + { + lock (staticLock) + return m_XmlRequest[m_ScriptEngine]; + } } public IScriptEngine[] ScriptEngines { - get { return m_ScriptEngines.ToArray(); } + get + { + lock (staticLock) + return m_ScriptEngines.ToArray(); + } } public AsyncCommandManager(IScriptEngine _ScriptEngine) @@ -112,29 +149,36 @@ namespace OpenSim.Region.ScriptEngine.Shared.Api m_ScriptEngine = _ScriptEngine; m_Scene = m_ScriptEngine.World; - if (m_Scenes.Count == 0) - ReadConfig(); - - if (!m_Scenes.Contains(m_Scene)) - m_Scenes.Add(m_Scene); - if (!m_ScriptEngines.Contains(m_ScriptEngine)) - m_ScriptEngines.Add(m_ScriptEngine); - - // Create instances of all plugins - if (!m_Dataserver.ContainsKey(m_ScriptEngine)) - m_Dataserver[m_ScriptEngine] = new Dataserver(this); - if (!m_Timer.ContainsKey(m_ScriptEngine)) - m_Timer[m_ScriptEngine] = new Timer(this); - if (!m_HttpRequest.ContainsKey(m_ScriptEngine)) - m_HttpRequest[m_ScriptEngine] = new HttpRequest(this); - if (!m_Listener.ContainsKey(m_ScriptEngine)) - m_Listener[m_ScriptEngine] = new Listener(this); - if (!m_SensorRepeat.ContainsKey(m_ScriptEngine)) - m_SensorRepeat[m_ScriptEngine] = new SensorRepeat(this); - if (!m_XmlRequest.ContainsKey(m_ScriptEngine)) - m_XmlRequest[m_ScriptEngine] = new XmlRequest(this); - - StartThread(); + // If there is more than one scene in the simulator or multiple script engines are used on the same region + // then more than one thread could arrive at this block of code simultaneously. However, it cannot be + // executed concurrently both because concurrent list operations are not thread-safe and because of other + // race conditions such as the later check of cmdHandlerThread == null. + lock (staticLock) + { + if (m_Scenes.Count == 0) + ReadConfig(); + + if (!m_Scenes.Contains(m_Scene)) + m_Scenes.Add(m_Scene); + if (!m_ScriptEngines.Contains(m_ScriptEngine)) + m_ScriptEngines.Add(m_ScriptEngine); + + // Create instances of all plugins + if (!m_Dataserver.ContainsKey(m_ScriptEngine)) + m_Dataserver[m_ScriptEngine] = new Dataserver(this); + if (!m_Timer.ContainsKey(m_ScriptEngine)) + m_Timer[m_ScriptEngine] = new Timer(this); + if (!m_HttpRequest.ContainsKey(m_ScriptEngine)) + m_HttpRequest[m_ScriptEngine] = new HttpRequest(this); + if (!m_Listener.ContainsKey(m_ScriptEngine)) + m_Listener[m_ScriptEngine] = new Listener(this); + if (!m_SensorRepeat.ContainsKey(m_ScriptEngine)) + m_SensorRepeat[m_ScriptEngine] = new SensorRepeat(this); + if (!m_XmlRequest.ContainsKey(m_ScriptEngine)) + m_XmlRequest[m_ScriptEngine] = new XmlRequest(this); + + StartThread(); + } } private static void StartThread() @@ -198,25 +242,28 @@ namespace OpenSim.Region.ScriptEngine.Shared.Api private static void DoOneCmdHandlerPass() { - // Check HttpRequests - m_HttpRequest[m_ScriptEngines[0]].CheckHttpRequests(); + lock (staticLock) + { + // Check HttpRequests + m_HttpRequest[m_ScriptEngines[0]].CheckHttpRequests(); - // Check XMLRPCRequests - m_XmlRequest[m_ScriptEngines[0]].CheckXMLRPCRequests(); + // Check XMLRPCRequests + m_XmlRequest[m_ScriptEngines[0]].CheckXMLRPCRequests(); - foreach (IScriptEngine s in m_ScriptEngines) - { - // Check Listeners - m_Listener[s].CheckListeners(); + foreach (IScriptEngine s in m_ScriptEngines) + { + // Check Listeners + m_Listener[s].CheckListeners(); - // Check timers - m_Timer[s].CheckTimerEvents(); + // Check timers + m_Timer[s].CheckTimerEvents(); - // Check Sensors - m_SensorRepeat[s].CheckSenseRepeaterEvents(); + // Check Sensors + m_SensorRepeat[s].CheckSenseRepeaterEvents(); - // Check dataserver - m_Dataserver[s].ExpireRequests(); + // Check dataserver + m_Dataserver[s].ExpireRequests(); + } } } @@ -229,32 +276,33 @@ namespace OpenSim.Region.ScriptEngine.Shared.Api { // m_log.DebugFormat("[ASYNC COMMAND MANAGER]: Removing facilities for script {0}", itemID); - // Remove a specific script + lock (staticLock) + { + // Remove dataserver events + m_Dataserver[engine].RemoveEvents(localID, itemID); - // Remove dataserver events - m_Dataserver[engine].RemoveEvents(localID, itemID); + // Remove from: Timers + m_Timer[engine].UnSetTimerEvents(localID, itemID); - // Remove from: Timers - m_Timer[engine].UnSetTimerEvents(localID, itemID); + // Remove from: HttpRequest + IHttpRequestModule iHttpReq = engine.World.RequestModuleInterface(); + if (iHttpReq != null) + iHttpReq.StopHttpRequestsForScript(itemID); - // Remove from: HttpRequest - IHttpRequestModule iHttpReq = engine.World.RequestModuleInterface(); - if (iHttpReq != null) - iHttpReq.StopHttpRequestsForScript(itemID); + IWorldComm comms = engine.World.RequestModuleInterface(); + if (comms != null) + comms.DeleteListener(itemID); - IWorldComm comms = engine.World.RequestModuleInterface(); - if (comms != null) - comms.DeleteListener(itemID); + IXMLRPC xmlrpc = engine.World.RequestModuleInterface(); + if (xmlrpc != null) + { + xmlrpc.DeleteChannels(itemID); + xmlrpc.CancelSRDRequests(itemID); + } - IXMLRPC xmlrpc = engine.World.RequestModuleInterface(); - if (xmlrpc != null) - { - xmlrpc.DeleteChannels(itemID); - xmlrpc.CancelSRDRequests(itemID); + // Remove Sensors + m_SensorRepeat[engine].UnSetSenseRepeaterEvents(localID, itemID); } - - // Remove Sensors - m_SensorRepeat[engine].UnSetSenseRepeaterEvents(localID, itemID); } /// @@ -264,10 +312,13 @@ namespace OpenSim.Region.ScriptEngine.Shared.Api /// public static SensorRepeat GetSensorRepeatPlugin(IScriptEngine engine) { - if (m_SensorRepeat.ContainsKey(engine)) - return m_SensorRepeat[engine]; - else - return null; + lock (staticLock) + { + if (m_SensorRepeat.ContainsKey(engine)) + return m_SensorRepeat[engine]; + else + return null; + } } /// @@ -277,10 +328,13 @@ namespace OpenSim.Region.ScriptEngine.Shared.Api /// public static Dataserver GetDataserverPlugin(IScriptEngine engine) { - if (m_Dataserver.ContainsKey(engine)) - return m_Dataserver[engine]; - else - return null; + lock (staticLock) + { + if (m_Dataserver.ContainsKey(engine)) + return m_Dataserver[engine]; + else + return null; + } } /// @@ -290,10 +344,13 @@ namespace OpenSim.Region.ScriptEngine.Shared.Api /// public static Timer GetTimerPlugin(IScriptEngine engine) { - if (m_Timer.ContainsKey(engine)) - return m_Timer[engine]; - else - return null; + lock (staticLock) + { + if (m_Timer.ContainsKey(engine)) + return m_Timer[engine]; + else + return null; + } } /// @@ -303,38 +360,44 @@ namespace OpenSim.Region.ScriptEngine.Shared.Api /// public static Listener GetListenerPlugin(IScriptEngine engine) { - if (m_Listener.ContainsKey(engine)) - return m_Listener[engine]; - else - return null; + lock (staticLock) + { + if (m_Listener.ContainsKey(engine)) + return m_Listener[engine]; + else + return null; + } } public static Object[] GetSerializationData(IScriptEngine engine, UUID itemID) { List data = new List(); - Object[] listeners = m_Listener[engine].GetSerializationData(itemID); - if (listeners.Length > 0) + lock (staticLock) { - data.Add("listener"); - data.Add(listeners.Length); - data.AddRange(listeners); - } + Object[] listeners = m_Listener[engine].GetSerializationData(itemID); + if (listeners.Length > 0) + { + data.Add("listener"); + data.Add(listeners.Length); + data.AddRange(listeners); + } - Object[] timers=m_Timer[engine].GetSerializationData(itemID); - if (timers.Length > 0) - { - data.Add("timer"); - data.Add(timers.Length); - data.AddRange(timers); - } + Object[] timers=m_Timer[engine].GetSerializationData(itemID); + if (timers.Length > 0) + { + data.Add("timer"); + data.Add(timers.Length); + data.AddRange(timers); + } - Object[] sensors = m_SensorRepeat[engine].GetSerializationData(itemID); - if (sensors.Length > 0) - { - data.Add("sensor"); - data.Add(sensors.Length); - data.AddRange(sensors); + Object[] sensors = m_SensorRepeat[engine].GetSerializationData(itemID); + if (sensors.Length > 0) + { + data.Add("sensor"); + data.Add(sensors.Length); + data.AddRange(sensors); + } } return data.ToArray(); @@ -359,20 +422,23 @@ namespace OpenSim.Region.ScriptEngine.Shared.Api idx+=len; + lock (staticLock) + { switch (type) { - case "listener": - m_Listener[engine].CreateFromData(localID, itemID, - hostID, item); - break; - case "timer": - m_Timer[engine].CreateFromData(localID, itemID, - hostID, item); - break; - case "sensor": - m_SensorRepeat[engine].CreateFromData(localID, - itemID, hostID, item); - break; + case "listener": + m_Listener[engine].CreateFromData(localID, itemID, + hostID, item); + break; + case "timer": + m_Timer[engine].CreateFromData(localID, itemID, + hostID, item); + break; + case "sensor": + m_SensorRepeat[engine].CreateFromData(localID, + itemID, hostID, item); + break; + } } } } -- cgit v1.1