From 4490020197c829d24a4dcd5bf9eec7e8bbd68809 Mon Sep 17 00:00:00 2001 From: "Justin Clark-Casey (justincc)" Date: Fri, 7 Dec 2012 00:47:04 +0000 Subject: [PATCH] Use a thread abort safe version of OpenMetaverse.DoubleDictionary with the aim of avoiding OpenSimulator problems due to script thread aborts. When an object is removed, its scripts are stopped and then the thread running them is aborted if stop takes too long. However, it appears that aborting a thread at just the wrong moment when it is obtaining a ReaderWriterLockSlim lock can leave this lock in an inconsistent state. One symptom of this is that mono leaps to 100% cpu and a vm thread dump reveals lots of threads waiting for a ReaderWriterLockSlim lock without any thread actually holding it. This is probably the same problem as encountered originally in commit 12cebb12 This commit looks to plaster this problem by putting lock obtaining methods inside finally blocks which should be uninterruptible by thread aborts. --- .../DoubleDictionaryThreadAbortSafe.cs | 508 ++++++++++++++++++ .../Region/Framework/Scenes/EntityManager.cs | 4 +- 2 files changed, 511 insertions(+), 1 deletion(-) create mode 100644 OpenSim/Framework/DoubleDictionaryThreadAbortSafe.cs diff --git a/OpenSim/Framework/DoubleDictionaryThreadAbortSafe.cs b/OpenSim/Framework/DoubleDictionaryThreadAbortSafe.cs new file mode 100644 index 0000000000..9056548a51 --- /dev/null +++ b/OpenSim/Framework/DoubleDictionaryThreadAbortSafe.cs @@ -0,0 +1,508 @@ +/* + * Copyright (c) 2008, openmetaverse.org, http://opensimulator.org/ + * All rights reserved. + * + * - Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * - Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * - Neither the name of the openmetaverse.org nor the names + * of its contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE + * POSSIBILITY OF SUCH DAMAGE. + */ + +using System; +using System.Threading; +using System.Collections.Generic; + +namespace OpenSim.Framework +{ + /// + /// A double dictionary that is thread abort safe. + /// + /// + /// This adapts OpenMetaverse.DoubleDictionary to be thread-abort safe by acquiring ReaderWriterLockSlim within + /// a finally section (which can't be interrupted by Thread.Abort()). + /// + public class DoubleDictionaryThreadAbortSafe + { + Dictionary Dictionary1; + Dictionary Dictionary2; + ReaderWriterLockSlim rwLock = new ReaderWriterLockSlim(); + + public DoubleDictionaryThreadAbortSafe() + { + Dictionary1 = new Dictionary(); + Dictionary2 = new Dictionary(); + } + + public DoubleDictionaryThreadAbortSafe(int capacity) + { + Dictionary1 = new Dictionary(capacity); + Dictionary2 = new Dictionary(capacity); + } + + public void Add(TKey1 key1, TKey2 key2, TValue value) + { + bool gotLock = false; + + try + { + // Avoid an asynchronous Thread.Abort() from possibly never existing an acquired lock by placing + // the acquision inside the main try. The inner finally block is needed because thread aborts cannot + // interrupt code in these blocks (hence gotLock is guaranteed to be set correctly). + try {} + finally + { + rwLock.EnterWriteLock(); + gotLock = true; + } + + if (Dictionary1.ContainsKey(key1)) + { + if (!Dictionary2.ContainsKey(key2)) + throw new ArgumentException("key1 exists in the dictionary but not key2"); + } + else if (Dictionary2.ContainsKey(key2)) + { + if (!Dictionary1.ContainsKey(key1)) + throw new ArgumentException("key2 exists in the dictionary but not key1"); + } + + Dictionary1[key1] = value; + Dictionary2[key2] = value; + } + finally + { + if (gotLock) + rwLock.ExitWriteLock(); + } + } + + public bool Remove(TKey1 key1, TKey2 key2) + { + bool success; + bool gotLock = false; + + try + { + // Avoid an asynchronous Thread.Abort() from possibly never existing an acquired lock by placing + // the acquision inside the main try. The inner finally block is needed because thread aborts cannot + // interrupt code in these blocks (hence gotLock is guaranteed to be set correctly). + try {} + finally + { + rwLock.EnterWriteLock(); + gotLock = true; + } + + Dictionary1.Remove(key1); + success = Dictionary2.Remove(key2); + } + finally + { + if (gotLock) + rwLock.ExitWriteLock(); + } + + return success; + } + + public bool Remove(TKey1 key1) + { + bool found = false; + bool gotLock = false; + + try + { + // Avoid an asynchronous Thread.Abort() from possibly never existing an acquired lock by placing + // the acquision inside the main try. The inner finally block is needed because thread aborts cannot + // interrupt code in these blocks (hence gotLock is guaranteed to be set correctly). + try {} + finally + { + rwLock.EnterWriteLock(); + gotLock = true; + } + + // This is an O(n) operation! + TValue value; + if (Dictionary1.TryGetValue(key1, out value)) + { + foreach (KeyValuePair kvp in Dictionary2) + { + if (kvp.Value.Equals(value)) + { + Dictionary1.Remove(key1); + Dictionary2.Remove(kvp.Key); + found = true; + break; + } + } + } + } + finally + { + if (gotLock) + rwLock.ExitWriteLock(); + } + + return found; + } + + public bool Remove(TKey2 key2) + { + bool found = false; + bool gotLock = false; + + try + { + // Avoid an asynchronous Thread.Abort() from possibly never existing an acquired lock by placing + // the acquision inside the main try. The inner finally block is needed because thread aborts cannot + // interrupt code in these blocks (hence gotLock is guaranteed to be set correctly). + try {} + finally + { + rwLock.EnterWriteLock(); + gotLock = true; + } + + // This is an O(n) operation! + TValue value; + if (Dictionary2.TryGetValue(key2, out value)) + { + foreach (KeyValuePair kvp in Dictionary1) + { + if (kvp.Value.Equals(value)) + { + Dictionary2.Remove(key2); + Dictionary1.Remove(kvp.Key); + found = true; + break; + } + } + } + } + finally + { + if (gotLock) + rwLock.ExitWriteLock(); + } + + return found; + } + + public void Clear() + { + bool gotLock = false; + + try + { + // Avoid an asynchronous Thread.Abort() from possibly never existing an acquired lock by placing + // the acquision inside the main try. The inner finally block is needed because thread aborts cannot + // interrupt code in these blocks (hence gotLock is guaranteed to be set correctly). + try {} + finally + { + rwLock.EnterWriteLock(); + gotLock = true; + } + + Dictionary1.Clear(); + Dictionary2.Clear(); + } + finally + { + if (gotLock) + rwLock.ExitWriteLock(); + } + } + + public int Count + { + get { return Dictionary1.Count; } + } + + public bool ContainsKey(TKey1 key) + { + return Dictionary1.ContainsKey(key); + } + + public bool ContainsKey(TKey2 key) + { + return Dictionary2.ContainsKey(key); + } + + public bool TryGetValue(TKey1 key, out TValue value) + { + bool success; + bool gotLock = false; + + try + { + // Avoid an asynchronous Thread.Abort() from possibly never existing an acquired lock by placing + // the acquision inside the main try. The inner finally block is needed because thread aborts cannot + // interrupt code in these blocks (hence gotLock is guaranteed to be set correctly). + try {} + finally + { + rwLock.EnterReadLock(); + gotLock = true; + } + + success = Dictionary1.TryGetValue(key, out value); + } + finally + { + if (gotLock) + rwLock.ExitReadLock(); + } + + return success; + } + + public bool TryGetValue(TKey2 key, out TValue value) + { + bool success; + bool gotLock = false; + + try + { + // Avoid an asynchronous Thread.Abort() from possibly never existing an acquired lock by placing + // the acquision inside the main try. The inner finally block is needed because thread aborts cannot + // interrupt code in these blocks (hence gotLock is guaranteed to be set correctly). + try {} + finally + { + rwLock.EnterReadLock(); + gotLock = true; + } + + success = Dictionary2.TryGetValue(key, out value); + } + finally + { + if (gotLock) + rwLock.ExitReadLock(); + } + + return success; + } + + public void ForEach(Action action) + { + bool gotLock = false; + + try + { + // Avoid an asynchronous Thread.Abort() from possibly never existing an acquired lock by placing + // the acquision inside the main try. The inner finally block is needed because thread aborts cannot + // interrupt code in these blocks (hence gotLock is guaranteed to be set correctly). + try {} + finally + { + rwLock.EnterReadLock(); + gotLock = true; + } + + foreach (TValue value in Dictionary1.Values) + action(value); + } + finally + { + if (gotLock) + rwLock.ExitReadLock(); + } + } + + public void ForEach(Action> action) + { + bool gotLock = false; + + try + { + // Avoid an asynchronous Thread.Abort() from possibly never existing an acquired lock by placing + // the acquision inside the main try. The inner finally block is needed because thread aborts cannot + // interrupt code in these blocks (hence gotLock is guaranteed to be set correctly). + try {} + finally + { + rwLock.EnterReadLock(); + gotLock = true; + } + + foreach (KeyValuePair entry in Dictionary1) + action(entry); + } + finally + { + if (gotLock) + rwLock.ExitReadLock(); + } + } + + public void ForEach(Action> action) + { + bool gotLock = false; + + try + { + // Avoid an asynchronous Thread.Abort() from possibly never existing an acquired lock by placing + // the acquision inside the main try. The inner finally block is needed because thread aborts cannot + // interrupt code in these blocks (hence gotLock is guaranteed to be set correctly). + try {} + finally + { + rwLock.EnterReadLock(); + gotLock = true; + } + + foreach (KeyValuePair entry in Dictionary2) + action(entry); + } + finally + { + if (gotLock) + rwLock.ExitReadLock(); + } + } + + public TValue FindValue(Predicate predicate) + { + bool gotLock = false; + + try + { + // Avoid an asynchronous Thread.Abort() from possibly never existing an acquired lock by placing + // the acquision inside the main try. The inner finally block is needed because thread aborts cannot + // interrupt code in these blocks (hence gotLock is guaranteed to be set correctly). + try {} + finally + { + rwLock.EnterReadLock(); + gotLock = true; + } + + foreach (TValue value in Dictionary1.Values) + { + if (predicate(value)) + return value; + } + } + finally + { + if (gotLock) + rwLock.ExitReadLock(); + } + + return default(TValue); + } + + public IList FindAll(Predicate predicate) + { + IList list = new List(); + bool gotLock = false; + + try + { + // Avoid an asynchronous Thread.Abort() from possibly never existing an acquired lock by placing + // the acquision inside the main try. The inner finally block is needed because thread aborts cannot + // interrupt code in these blocks (hence gotLock is guaranteed to be set correctly). + try {} + finally + { + rwLock.EnterReadLock(); + gotLock = true; + } + + foreach (TValue value in Dictionary1.Values) + { + if (predicate(value)) + list.Add(value); + } + } + finally + { + if (gotLock) + rwLock.ExitReadLock(); + } + + return list; + } + + public int RemoveAll(Predicate predicate) + { + IList list = new List(); + bool gotUpgradeableLock = false; + + try + { + // Avoid an asynchronous Thread.Abort() from possibly never existing an acquired lock by placing + // the acquision inside the main try. The inner finally block is needed because thread aborts cannot + // interrupt code in these blocks (hence gotLock is guaranteed to be set correctly). + try {} + finally + { + rwLock.EnterUpgradeableReadLock(); + gotUpgradeableLock = true; + } + + foreach (KeyValuePair kvp in Dictionary1) + { + if (predicate(kvp.Value)) + list.Add(kvp.Key); + } + + IList list2 = new List(list.Count); + foreach (KeyValuePair kvp in Dictionary2) + { + if (predicate(kvp.Value)) + list2.Add(kvp.Key); + } + + bool gotWriteLock = false; + + try + { + try {} + finally + { + rwLock.EnterUpgradeableReadLock(); + gotWriteLock = true; + } + + for (int i = 0; i < list.Count; i++) + Dictionary1.Remove(list[i]); + + for (int i = 0; i < list2.Count; i++) + Dictionary2.Remove(list2[i]); + } + finally + { + if (gotWriteLock) + rwLock.ExitWriteLock(); + } + } + finally + { + if (gotUpgradeableLock) + rwLock.ExitUpgradeableReadLock(); + } + + return list.Count; + } + } +} \ No newline at end of file diff --git a/OpenSim/Region/Framework/Scenes/EntityManager.cs b/OpenSim/Region/Framework/Scenes/EntityManager.cs index b788a3ca08..7181313fbe 100644 --- a/OpenSim/Region/Framework/Scenes/EntityManager.cs +++ b/OpenSim/Region/Framework/Scenes/EntityManager.cs @@ -31,6 +31,7 @@ using System.Collections.Generic; using System.Reflection; using log4net; using OpenMetaverse; +using OpenSim.Framework; namespace OpenSim.Region.Framework.Scenes { @@ -38,7 +39,8 @@ namespace OpenSim.Region.Framework.Scenes { // private static readonly ILog m_log = LogManager.GetLogger(MethodBase.GetCurrentMethod().DeclaringType); - private readonly DoubleDictionary m_entities = new DoubleDictionary(); + private readonly DoubleDictionaryThreadAbortSafe m_entities + = new DoubleDictionaryThreadAbortSafe(); public int Count {