From 21a09ad3ad42b24bce4fc04c6bcd6f7d9a80af08 Mon Sep 17 00:00:00 2001
From: Justin Clark-Casey (justincc)
Date: Tue, 16 Jul 2013 21:54:00 +0100
Subject: Revert "MSDN documentation is unclear about whether exiting a lock()
block will trigger a Monitor.Wait() to exit, so avoid some locks that don't
actually affect the state of the internal queues in the BlockingQueue class."
This reverts commit 42e2a0d66eaa7e322bce817e9e2cc9a288de167b
Reverting because unfortunately this introduces race conditions because Contains(), Count() and GetQueueArray() may now end up returning the wrong result if another thread performs a simultaneous update on m_queue.
Code such as PollServiceRequestManager.Stop() relies on the count being correct otherwise a request may be lost.
Also, though some of the internal queue methods do not affect state, they are not thread-safe and could return the wrong result generating the same problem
lock() generates Monitor.Enter() and Monitor.Exit() under the covers. Monitor.Exit() does not cause Monitor.Wait() to exist, only Pulse() and PulseAll() will do this
Reverted with agreement.
---
OpenSim/Framework/BlockingQueue.cs | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
(limited to 'OpenSim/Framework')
diff --git a/OpenSim/Framework/BlockingQueue.cs b/OpenSim/Framework/BlockingQueue.cs
index cc016b0..3658161 100644
--- a/OpenSim/Framework/BlockingQueue.cs
+++ b/OpenSim/Framework/BlockingQueue.cs
@@ -58,7 +58,7 @@ namespace OpenSim.Framework
{
lock (m_queueSync)
{
- while (m_queue.Count < 1 && m_pqueue.Count < 1)
+ if (m_queue.Count < 1 && m_pqueue.Count < 1)
{
Monitor.Wait(m_queueSync);
}
@@ -91,9 +91,6 @@ namespace OpenSim.Framework
public bool Contains(T item)
{
- if (m_queue.Count < 1 && m_pqueue.Count < 1)
- return false;
-
lock (m_queueSync)
{
if (m_pqueue.Contains(item))
@@ -104,14 +101,14 @@ namespace OpenSim.Framework
public int Count()
{
- return m_queue.Count+m_pqueue.Count;
+ lock (m_queueSync)
+ {
+ return m_queue.Count+m_pqueue.Count;
+ }
}
public T[] GetQueueArray()
{
- if (m_queue.Count < 1 && m_pqueue.Count < 1)
- return new T[0];
-
lock (m_queueSync)
{
return m_queue.ToArray();
--
cgit v1.1
From 50b8ab60f2d4d8a2cc7024012fc1333be7635276 Mon Sep 17 00:00:00 2001
From: Justin Clark-Casey (justincc)
Date: Tue, 16 Jul 2013 23:00:07 +0100
Subject: Revert "Revert "MSDN documentation is unclear about whether exiting a
lock() block will trigger a Monitor.Wait() to exit, so avoid some locks that
don't actually affect the state of the internal queues in the BlockingQueue
class.""
This reverts commit 21a09ad3ad42b24bce4fc04c6bcd6f7d9a80af08.
After more analysis and discussion, it is apparant that the Count(), Contains() and GetQueueArray() cannot be made thread-safe anyway without external locking
And this change appears to have a positive impact on performance.
I still believe that Monitor.Exit() will not release any thread for Monitor.Wait(), as per http://msdn.microsoft.com/en-gb/library/vstudio/system.threading.monitor.exit%28v=vs.100%29.aspx
so this should in theory make no difference, though mono implementation issues could possibly be coming into play.
---
OpenSim/Framework/BlockingQueue.cs | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
(limited to 'OpenSim/Framework')
diff --git a/OpenSim/Framework/BlockingQueue.cs b/OpenSim/Framework/BlockingQueue.cs
index 3658161..cc016b0 100644
--- a/OpenSim/Framework/BlockingQueue.cs
+++ b/OpenSim/Framework/BlockingQueue.cs
@@ -58,7 +58,7 @@ namespace OpenSim.Framework
{
lock (m_queueSync)
{
- if (m_queue.Count < 1 && m_pqueue.Count < 1)
+ while (m_queue.Count < 1 && m_pqueue.Count < 1)
{
Monitor.Wait(m_queueSync);
}
@@ -91,6 +91,9 @@ namespace OpenSim.Framework
public bool Contains(T item)
{
+ if (m_queue.Count < 1 && m_pqueue.Count < 1)
+ return false;
+
lock (m_queueSync)
{
if (m_pqueue.Contains(item))
@@ -101,14 +104,14 @@ namespace OpenSim.Framework
public int Count()
{
- lock (m_queueSync)
- {
- return m_queue.Count+m_pqueue.Count;
- }
+ return m_queue.Count+m_pqueue.Count;
}
public T[] GetQueueArray()
{
+ if (m_queue.Count < 1 && m_pqueue.Count < 1)
+ return new T[0];
+
lock (m_queueSync)
{
return m_queue.ToArray();
--
cgit v1.1
From cbc3576ee2ee0afc7cbf66d4d782dfaee9bbdcda Mon Sep 17 00:00:00 2001
From: Justin Clark-Casey (justincc)
Date: Tue, 16 Jul 2013 23:14:53 +0100
Subject: minor: Add warning method doc about possibly inconsistent results
returned from BlockingQueue.Contains(), Count() and GetQueueArray()
---
OpenSim/Framework/BlockingQueue.cs | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
(limited to 'OpenSim/Framework')
diff --git a/OpenSim/Framework/BlockingQueue.cs b/OpenSim/Framework/BlockingQueue.cs
index cc016b0..aef1192 100644
--- a/OpenSim/Framework/BlockingQueue.cs
+++ b/OpenSim/Framework/BlockingQueue.cs
@@ -89,6 +89,12 @@ namespace OpenSim.Framework
}
}
+ ///
+ /// Indicate whether this queue contains the given item.
+ ///
+ ///
+ /// This method is not thread-safe. Do not rely on the result without consistent external locking.
+ ///
public bool Contains(T item)
{
if (m_queue.Count < 1 && m_pqueue.Count < 1)
@@ -102,11 +108,23 @@ namespace OpenSim.Framework
}
}
+ ///
+ /// Return a count of the number of requests on this queue.
+ ///
+ ///
+ /// This method is not thread-safe. Do not rely on the result without consistent external locking.
+ ///
public int Count()
{
- return m_queue.Count+m_pqueue.Count;
+ return m_queue.Count + m_pqueue.Count;
}
+ ///
+ /// Return the array of items on this queue.
+ ///
+ ///
+ /// This method is not thread-safe. Do not rely on the result without consistent external locking.
+ ///
public T[] GetQueueArray()
{
if (m_queue.Count < 1 && m_pqueue.Count < 1)
--
cgit v1.1