diff --git a/indra/llcommon/llqueuedthread.cpp b/indra/llcommon/llqueuedthread.cpp index af73f6236..31bb145fe 100644 --- a/indra/llcommon/llqueuedthread.cpp +++ b/indra/llcommon/llqueuedthread.cpp @@ -264,7 +264,7 @@ bool LLQueuedThread::waitForResult(LLQueuedThread::handle_t handle, bool auto_co { done = true; // request does not exist } - else if (req->getStatus() == STATUS_COMPLETE) + else if (req->getStatus() == STATUS_COMPLETE && !(req->getFlags() & FLAG_LOCKED)) { res = true; if (auto_complete) @@ -372,9 +372,17 @@ bool LLQueuedThread::completeRequest(handle_t handle) #if _DEBUG // llinfos << llformat("LLQueuedThread::Completed req [%08d]",handle) << llendl; #endif - mRequestHash.erase(handle); - req->deleteRequest(); -// check(); + if (!(req->getFlags() & FLAG_LOCKED)) + { + mRequestHash.erase(handle); + req->deleteRequest(); +// check(); + } + else + { + // Cause deletion immediately after FLAG_LOCKED is released. + req->setFlags(FLAG_AUTO_COMPLETE); + } res = true; } unlockData(); @@ -421,12 +429,44 @@ S32 LLQueuedThread::processNextRequest() if ((req->getFlags() & FLAG_ABORT) || (mStatus == QUITTING)) { req->setStatus(STATUS_ABORTED); + // Unlock, because we can't call finishRequest() while keeping this lock: + // generateHandle() takes this lock too and is called while holding a lock + // (ie LLTextureFetchWorker::mWorkMutex) that finishRequest will lock too, + // causing a dead lock. + // Although a complete rewrite of LLQueuedThread is in order because it's + // far from robust the way it handles it's locking; the following rationale + // at least makes plausible that releasing the lock here SHOULD work if + // the original coder didn't completely fuck up: if before the QueuedRequest + // req was only accessed while keeping the lock, then it still should + // never happen that another thread is waiting for this lock in order to + // access the QueuedRequest: a few lines lower we delete it. + // In other words, if directly after releasing this lock another thread + // would access req, then that can only happen by finding it again in + // either mRequestQueue or mRequestHash. We already deleted it from the + // first, so this access would have to happen by finding it in mRequestHash. + // Such access happens in the following functions: + // 1) LLQueuedThread::shutdown -- but it does that anyway, as it doesn't use any locking. + // 2) LLQueuedThread::generateHandle -- might skip our handle while before it would block until we deleted it. Skipping it is actually better. + // 3) LLQueuedThread::waitForResult -- this now doesn't touch the req when it has the flag FLAG_LOCKED set. + // 4) LLQueuedThread::getRequest -- whereever this is used, FLAG_LOCKED is tested before using the req. + // 5) LLQueuedThread::getRequestStatus -- this is a read-only operation on the status, which should never be changed from finishRequest(). + // 6) LLQueuedThread::abortRequest -- it doesn't seem to hurt to add flags (if this happens at all), while calling finishRequest(). + // 7) LLQueuedThread::setFlags -- same. + // 8) LLQueuedThread::setPriority -- doesn't access req with status STATUS_ABORTED, STATUS_COMPLETE or STATUS_INPROGRESS. + // 9) LLQueuedThread::completeRequest -- now sets FLAG_AUTO_COMPLETE instead of deleting the req, if FLAG_LOCKED is set, so that deletion happens here when finishRequest returns. + req->setFlags(FLAG_LOCKED); + unlockData(); req->finishRequest(false); - if (req->getFlags() & FLAG_AUTO_COMPLETE) + lockData(); + req->resetFlags(FLAG_LOCKED); + if ((req->getFlags() & FLAG_AUTO_COMPLETE)) { + req->resetFlags(FLAG_AUTO_COMPLETE); mRequestHash.erase(req); - req->deleteRequest(); // check(); + unlockData(); + req->deleteRequest(); + lockData(); } continue; } @@ -453,14 +493,23 @@ S32 LLQueuedThread::processNextRequest() { lockData(); req->setStatus(STATUS_COMPLETE); - req->finishRequest(true); - if (req->getFlags() & FLAG_AUTO_COMPLETE) - { - mRequestHash.erase(req); - req->deleteRequest(); -// check(); - } + req->setFlags(FLAG_LOCKED); unlockData(); + req->finishRequest(true); + if ((req->getFlags() & FLAG_AUTO_COMPLETE)) + { + lockData(); + req->resetFlags(FLAG_AUTO_COMPLETE); + mRequestHash.erase(req); +// check(); + req->resetFlags(FLAG_LOCKED); + unlockData(); + req->deleteRequest(); + } + else + { + req->resetFlags(FLAG_LOCKED); + } } else { diff --git a/indra/llcommon/llqueuedthread.h b/indra/llcommon/llqueuedthread.h index 2680072bc..6a3dc4078 100644 --- a/indra/llcommon/llqueuedthread.h +++ b/indra/llcommon/llqueuedthread.h @@ -72,7 +72,8 @@ public: enum flags_t { FLAG_AUTO_COMPLETE = 1, FLAG_AUTO_DELETE = 2, // child-class dependent - FLAG_ABORT = 4 + FLAG_ABORT = 4, + FLAG_LOCKED = 8 }; typedef U32 handle_t; @@ -122,6 +123,10 @@ public: // NOTE: flags are |'d mFlags |= flags; } + void resetFlags(U32 flags) + { + mFlags &= ~flags; + } virtual bool processRequest() = 0; // Return true when request has completed virtual void finishRequest(bool completed); // Always called from thread after request has completed or aborted diff --git a/indra/llcommon/llworkerthread.cpp b/indra/llcommon/llworkerthread.cpp index 96198b6e3..a5e0844c6 100644 --- a/indra/llcommon/llworkerthread.cpp +++ b/indra/llcommon/llworkerthread.cpp @@ -226,7 +226,8 @@ LLWorkerClass::~LLWorkerClass() llerrs << "LLWorkerClass destroyed with stale work handle" << llendl; } if (workreq->getStatus() != LLWorkerThread::STATUS_ABORTED && - workreq->getStatus() != LLWorkerThread::STATUS_COMPLETE) + workreq->getStatus() != LLWorkerThread::STATUS_COMPLETE && + !(workreq->getFlags() & LLWorkerThread::FLAG_LOCKED)) { llerrs << "LLWorkerClass destroyed with active worker! Worker Status: " << workreq->getStatus() << llendl; } @@ -350,14 +351,10 @@ bool LLWorkerClass::checkWork(bool aborting) } LLQueuedThread::status_t status = workreq->getStatus(); - if (status == LLWorkerThread::STATUS_ABORTED) + if (status == LLWorkerThread::STATUS_COMPLETE || status == LLWorkerThread::STATUS_ABORTED) { - complete = true; - abort = true; - } - else if (status == LLWorkerThread::STATUS_COMPLETE) - { - complete = true; + complete = !(workreq->getFlags() & LLWorkerThread::FLAG_LOCKED); + abort = status == LLWorkerThread::STATUS_ABORTED; } else { diff --git a/indra/llvfs/llvfile.cpp b/indra/llvfs/llvfile.cpp index 419d1b76c..d381a01f1 100644 --- a/indra/llvfs/llvfile.cpp +++ b/indra/llvfs/llvfile.cpp @@ -175,7 +175,7 @@ BOOL LLVFile::isReadComplete() { LLVFSThread::Request* req = (LLVFSThread::Request*)sVFSThread->getRequest(mHandle); LLVFSThread::status_t status = req->getStatus(); - if (status == LLVFSThread::STATUS_COMPLETE) + if (status == LLVFSThread::STATUS_COMPLETE && !(req->getFlags() & LLVFSThread::FLAG_LOCKED)) { mBytesRead = req->getBytesRead(); mPosition += mBytesRead;