Aleric Inglewood: SNOW-196 Special case for TextureFetch race condition check.

Where the current UUID was just removed from mRequestMap, thus avoiding to the
need for the problematic lock completely. The case that we add something to
mNetworkQueue while it was just removed from mRequestMap is corrected by
LLTextureFetch::sendRequestListToSimulators anyway.

3 files changed, 20 insertions, 65 deletions

diff --git a/linden/indra/llcommon/llthread.cpp b/linden/indra/llcommon/llthread.cpp
index 60ccd5c..692d6c4 100644
--- a/linden/indra/llcommon/llthread.cpp
+++ b/linden/indra/llcommon/llthread.cpp
@@ -283,7 +283,6 @@ LLMutex::LLMutex(apr_pool_t *poolp) :
         apr_thread_mutex_create(&mAPRMutexp, APR_THREAD_MUTEX_UNNESTED, mAPRPoolp);
 }
 
-
 LLMutex::~LLMutex()
 {
 #if _DEBUG
@@ -297,40 +296,14 @@ LLMutex::~LLMutex()
         }
 }
 
-
-void LLMutex::lock()
-{
-        apr_thread_mutex_lock(mAPRMutexp);
-}
-
-void LLMutex::unlock()
-{
-        apr_thread_mutex_unlock(mAPRMutexp);
-}
-
 bool LLMutex::isLocked()
 {
-        apr_status_t status = apr_thread_mutex_trylock(mAPRMutexp);
-        if (APR_STATUS_IS_EBUSY(status))
+        if (!tryLock())
         {
                 return true;
         }
-        else
-        {
-                apr_thread_mutex_unlock(mAPRMutexp);
-                return false;
-        }
-}
-
-//trys to grab a lock and if sucessful returns true
-bool LLMutex::tryLock()
-{
-        apr_status_t status = apr_thread_mutex_trylock(mAPRMutexp);
-        if (APR_STATUS_IS_EBUSY(status))
-        {       
-                return false;
-        }
-        return true;
+        apr_thread_mutex_unlock(mAPRMutexp);
+        return false;
 }
 
 //============================================================================
@@ -343,14 +316,12 @@ LLCondition::LLCondition(apr_pool_t *poolp) :
         apr_thread_cond_create(&mAPRCondp, mAPRPoolp);
 }
 
-
 LLCondition::~LLCondition()
 {
         apr_thread_cond_destroy(mAPRCondp);
         mAPRCondp = NULL;
 }
 
-
 void LLCondition::wait()
 {
         apr_thread_cond_wait(mAPRCondp, mAPRMutexp);
diff --git a/linden/indra/llcommon/llthread.h b/linden/indra/llcommon/llthread.h
index d773fa4..ed76a3b 100644
--- a/linden/indra/llcommon/llthread.h
+++ b/linden/indra/llcommon/llthread.h
@@ -131,10 +131,12 @@ public:
         LLMutex(apr_pool_t *apr_poolp); // NULL pool constructs a new pool for the mutex
         ~LLMutex();
         
-        void lock();            // blocks
-        void unlock();
+        void lock() { apr_thread_mutex_lock(mAPRMutexp); }
+        void unlock() { apr_thread_mutex_unlock(mAPRMutexp); }
+        // Returns true if lock was obtained successfully.
+        bool tryLock() { return !APR_STATUS_IS_EBUSY(apr_thread_mutex_trylock(mAPRMutexp)); }
+
         bool isLocked();        // non-blocking, but does do a lock/unlock so not free
-        bool tryLock();         //non-blocking, but not free, returns true if grabed lock
 
 protected:
         apr_thread_mutex_t *mAPRMutexp;
diff --git a/linden/indra/newview/lltexturefetch.cpp b/linden/indra/newview/lltexturefetch.cpp
index 3a5665c..62b4c8b 100644
--- a/linden/indra/newview/lltexturefetch.cpp
+++ b/linden/indra/newview/lltexturefetch.cpp
@@ -744,26 +744,17 @@ bool LLTextureFetchWorker::doWork(S32 param)
                 }
                 else if (mSentRequest == UNSENT)
                 {
-                        if(mFetcher->mQueueMutex.tryLock())
-                        {
-                                // Add this to the network queue and sit here.
-                                // LLTextureFetch::update() will send off a request which will change our state
-                                mRequestedSize = mDesiredSize;
-                                mRequestedDiscard = mDesiredDiscard;
-                                mSentRequest = QUEUED;  
-                                mFetcher->addToNetworkQueue(this);
-                                setPriority(LLWorkerThread::PRIORITY_LOW | mWorkPriority);
-                                mFetcher->mQueueMutex.unlock();
-                        }
+                        // Add this to the network queue and sit here.
+                        // LLTextureFetch::update() will send off a request which will change our state
+                        mRequestedSize = mDesiredSize;
+                        mRequestedDiscard = mDesiredDiscard;
+                        mSentRequest = QUEUED;
+                        mFetcher->addToNetworkQueue(this);
+                        setPriority(LLWorkerThread::PRIORITY_LOW | mWorkPriority);
                         return false;
                 }
                 else
                 {
-                        // Shouldn't need to do anything here
-                        //llassert_always(mFetcher->mNetworkQueue.find(mID) != mFetcher->mNetworkQueue.end());
-                        // Make certain this is in the network queue
-                        //mFetcher->addToNetworkQueue(this);
-                        //setPriority(LLWorkerThread::PRIORITY_LOW | mWorkPriority);
                         return false;
                 }
         }
@@ -1473,23 +1464,12 @@ void LLTextureFetch::deleteRequest(const LLUUID& id, bool cancel)
         }
 }
 
-// Need to hold mQueueMutex when entering this function SNOW-196
-// Snowglobe ToDo fix holding mQueueMutex over the entire function, we only need
-// it for the mRequestMap access
 // protected
 void LLTextureFetch::addToNetworkQueue(LLTextureFetchWorker* worker)
 {
-        bool is_worker_in_request_map = (mRequestMap.find(worker->mID) != mRequestMap.end());
-
         LLMutexLock lock(&mNetworkQueueMutex);
-        if (is_worker_in_request_map)
-        {
-                // only add to the queue if in the request map
-                // i.e. a delete has not been requested
-                mNetworkQueue.insert(worker->mID);
-        }
-        for (cancel_queue_t::iterator iter1 = mCancelQueue.begin();
-                 iter1 != mCancelQueue.end(); ++iter1)
+        mNetworkQueue.insert(worker->mID);
+        for (cancel_queue_t::iterator iter1 = mCancelQueue.begin(); iter1 != mCancelQueue.end(); ++iter1)
         {
                 iter1->second.erase(worker->mID);
         }
@@ -1715,8 +1695,10 @@ void LLTextureFetch::sendRequestListToSimulators()
                 LLTextureFetchWorker* req = getWorker(*curiter);
                 if (!req)
                 {
+                        // This happens when a request was removed from mRequestMap in a race
+                        // with adding it to mNetworkQueue by doWork (see SNOW-196).
                         mNetworkQueue.erase(curiter);
-                        continue; // paranoia
+                        continue;
                 }
                 llassert(req->mState == LLTextureFetchWorker::LOAD_FROM_NETWORK || LLTextureFetchWorker::LOAD_FROM_SIMULATOR);
                 if ((req->mState != LLTextureFetchWorker::LOAD_FROM_NETWORK) &&