VirtualBox

Changeset 48908 in vbox for trunk


Ignore:
Timestamp:
Oct 5, 2013 11:02:10 PM (11 years ago)
Author:
vboxsync
Message:

UIThreadPool: Simplified the locking, avoiding several races. Fixed accounting race between thread stopping and tasks being enqueued in a busy pool. Try prevent sltHandleWorkerFinished racing the destructor and accessing freed memory, and even if it does, changed the method such that it doesn't actually access the pool instance data at all.

Location:
trunk/src/VBox/Frontends/VirtualBox/src/globals
Files:
2 edited

Legend:

Unmodified
Added
Removed

  • trunk/src/VBox/Frontends/VirtualBox/src/globals/UIThreadPool.cpp

    r48894 r48908  
    11/* $Id$ */
    22/** @file
    3  *
    4  * VBox frontends: Qt GUI ("VirtualBox"):
    5  * UIThreadPool class implementation
     3 * VBox Qt GUI - UIThreadPool and UITask class implementation.
    64 */
    75
    … …  
    3230#include <VBox/sup.h>
    3331
    34 /* Worker-thread prototype.
    35  * Performs COM-related GUI tasks in separate thread. */
     32/** @todo r=bird: this code was racy, inefficient and buggy in it's first
     33 *        incarnation.  What is required from RTReqPool for it to replace the
     34 *        internals here?  COM init/term hooks, sure. Does the threads need to
     35 *        be QThreads? */
     36
     37
     38/**
     39 * COM capable worker thread for the UIThreadPool.
     40 */
    3641class UIThreadWorker : public QThread
    3742{
    … …  
    4853    UIThreadWorker(UIThreadPool *pPool, int iIndex);
    4954
    50     /* API: Busy stuff: */
    51     bool isBusy() const;
    52     void setBusy(bool fBusy);
     55    int getIndex() const { return m_iIndex; }
     56
     57    /** Disables sigFinished.  For optimizing pool termination. */
     58    void setNoFinishedSignal()
     59    {
     60        m_fNoFinishedSignal = true;
     61    }
    5362
    5463private:
    … …  
    5968    /* Variables: General stuff: */
    6069    UIThreadPool *m_pPool;
     70
     71    /** The index into UIThreadPool::m_workers. */
    6172    int m_iIndex;
    62 
    63     /* Variables: Busy stuff: */
    64     bool m_fBusy;
    65     mutable QMutex m_busyLocker;
     73    /** Indicates whether sigFinished should be emitted or not. */
     74    bool m_fNoFinishedSignal;
    6675};
    6776
    6877
    69 UIThreadPool::UIThreadPool(ulong uWorkerCount /* = 3*/, ulong uWorkerIdleTimeout /* = 5000*/)
    70     : m_workers(uWorkerCount /* maximum worker count */)
    71     , m_uIdleTimeout(uWorkerIdleTimeout) /* time for worker idle timeout */
     78UIThreadPool::UIThreadPool(ulong cMaxWorkers/* = 3*/, ulong cMsWorkerIdleTimeout /* = 5000*/)
     79    : m_workers(cMaxWorkers)
     80    , m_cMsIdleTimeout(cMsWorkerIdleTimeout)
     81    , m_cWorkers(0)
     82    , m_cIdleWorkers(0)
    7283    , m_fTerminating(false) /* termination status */
     84    , m_everythingLocker(QMutex::Recursive)
    7385{
    7486}
    … …  
    7688UIThreadPool::~UIThreadPool()
    7789{
    78     /* Set termination status: */
    79     setTerminating(true);
     90    /* Set termination status and alert all idle worker threads: */
     91    setTerminating();
    8092
    8193    /* Cleanup all the workers: */
    82     m_taskCondition.wakeAll();
    83     for (int iWorkerIndex = 0; iWorkerIndex < m_workers.size(); ++iWorkerIndex)
    84         cleanupWorker(iWorkerIndex);
     94    m_everythingLocker.lock(); /* paranoia */
     95    for (int idxWorker = 0; idxWorker < m_workers.size(); ++idxWorker)
     96    {
     97        UIThreadWorker *pWorker = m_workers[idxWorker];
     98        m_workers[idxWorker] = NULL;
     99
     100        /* Clean up the worker, if there was one. */
     101        if (pWorker)
     102        {
     103            m_cWorkers--;
     104            m_everythingLocker.unlock();
     105
     106            pWorker->wait();
     107
     108            m_everythingLocker.lock();
     109            delete pWorker;
     110        }
     111    }
     112
     113    m_everythingLocker.unlock();
    85114}
    86115
    87116void UIThreadPool::enqueueTask(UITask *pTask)
    88117{
     118    Assert(!isTerminating());
     119
    89120    /* Prepare task: */
    90121    connect(pTask, SIGNAL(sigComplete(UITask*)), this, SLOT(sltHandleTaskComplete(UITask*)), Qt::QueuedConnection);
    91122
    92     /* Post task into queue: */
    93     m_taskLocker.lock();
     123    m_everythingLocker.lock();
     124
     125    /* Put the task onto the queue: */
    94126    m_tasks.enqueue(pTask);
    95     m_taskLocker.unlock();
    96 
    97     /* Search for the first 'not yet created' worker or wake idle if exists: */
    98     int iFirstNotYetCreatedWorkerIndex = -1;
    99     for (int iWorkerIndex = 0; iWorkerIndex < m_workers.size(); ++iWorkerIndex)
    100     {
    101         /* Remember first 'not yet created' worker: */
    102         if (iFirstNotYetCreatedWorkerIndex == -1 && !m_workers[iWorkerIndex])
    103             iFirstNotYetCreatedWorkerIndex = iWorkerIndex;
    104         /* But if worker 'not yet created' or 'busy' now, just skip it: */
    105         if (!m_workers[iWorkerIndex] || m_workers[iWorkerIndex]->isBusy())
    106             continue;
    107         /* If we found idle worker, wake it up: */
     127
     128    /* Wake up an idle worker if we got one: */
     129    if (m_cIdleWorkers > 0)
     130    {
    108131        m_taskCondition.wakeOne();
    109         return;
    110     }
    111 
    112     /* Should we create new worker? */
    113     if (iFirstNotYetCreatedWorkerIndex != -1)
    114     {
    115         /* Prepare worker: */
    116         UIThreadWorker *pWorker = new UIThreadWorker(this, iFirstNotYetCreatedWorkerIndex);
    117         connect(pWorker, SIGNAL(sigFinished(UIThreadWorker*)), this, SLOT(sltHandleWorkerFinished(UIThreadWorker*)), Qt::QueuedConnection);
    118         m_workers[iFirstNotYetCreatedWorkerIndex] = pWorker;
    119         /* And start it: */
    120         pWorker->start();
    121     }
     132    }
     133    /* No idle worker threads, should we create a new one? */
     134    else if (m_cWorkers < m_workers.size())
     135    {
     136        /* Find free slot. */
     137        int idxFirstUnused = m_workers.size();
     138        while (idxFirstUnused-- > 0)
     139            if (m_workers[idxFirstUnused] == NULL)
     140            {
     141                /* Prepare the new worker: */
     142                UIThreadWorker *pWorker = new UIThreadWorker(this, idxFirstUnused);
     143                connect(pWorker, SIGNAL(sigFinished(UIThreadWorker*)), this,
     144                        SLOT(sltHandleWorkerFinished(UIThreadWorker*)), Qt::QueuedConnection);
     145                m_workers[idxFirstUnused] = pWorker;
     146                m_cWorkers++;
     147
     148                /* And start it: */
     149                pWorker->start();
     150                break;
     151            }
     152    }
     153    /* else: wait for some worker to complete whatever it's busy with and jump to it. */
     154
     155    m_everythingLocker.unlock();
    122156}
    123157
    … …  
    125159{
    126160    /* Acquire termination-flag: */
    127     bool fTerminating = false;
    128     {
    129         QMutexLocker lock(&m_terminationLocker);
    130         fTerminating = m_fTerminating;
    131         Q_UNUSED(lock);
    132     }
     161    QMutexLocker lock(&m_everythingLocker);
     162    bool fTerminating = m_fTerminating;
     163    lock.unlock();
     164
    133165    return fTerminating;
    134166}
    135167
    136 void UIThreadPool::setTerminating(bool fTerminating)
    137 {
    138     /* Assign termination-flag: */
    139     QMutexLocker lock(&m_terminationLocker);
    140     m_fTerminating = fTerminating;
    141     Q_UNUSED(lock);
     168void UIThreadPool::setTerminating()
     169{
     170    m_everythingLocker.lock();
     171
     172    /* Indicate that we're terminating: */
     173    m_fTerminating = true;
     174
     175    /* Tell all threads to NOT queue any termination signals: */
     176    for (int idxWorker = 0; idxWorker < m_workers.size(); ++idxWorker)
     177    {
     178        UIThreadWorker *pWorker = m_workers[idxWorker];
     179        if (pWorker)
     180            pWorker->setNoFinishedSignal();
     181    }
     182
     183    /* Wake up all idle worker threads: */
     184    m_taskCondition.wakeAll();
     185
     186    m_everythingLocker.unlock();
    142187}
    143188
    144189UITask* UIThreadPool::dequeueTask(UIThreadWorker *pWorker)
    145190{
    146     /* Prepare task: */
    147     UITask *pTask = 0;
    148 
    149     /* Lock task locker: */
    150     m_taskLocker.lock();
    151 
    152     /* Try to dequeue task: */
    153     if (!m_tasks.isEmpty())
    154         pTask = m_tasks.dequeue();
    155 
    156     /* Make sure we have a task or outdated: */
    157     bool fOutdated = false;
    158     while (!isTerminating() && !pTask && !fOutdated)
    159     {
    160         /* Mark thread as not busy: */
    161         pWorker->setBusy(false);
    162 
    163         /* Wait for <m_uIdleTimeout> milli-seconds for the next task,
    164          * this issue will temporary unlock <m_taskLocker>: */
    165         fOutdated = !m_taskCondition.wait(&m_taskLocker, m_uIdleTimeout);
    166 
    167         /* Mark thread as busy again: */
    168         pWorker->setBusy(true);
    169 
    170         /* Try to dequeue task again: */
     191    /*
     192     * Dequeue a task, watching out for terminations. For opimal efficiency in
     193     * enqueueTask() we keep count of idle threads.
     194     *
     195     * If the wait times out, we'll return NULL and terminate the thread.
     196     */
     197    m_everythingLocker.lock();
     198
     199    bool fIdleTimedOut = false;
     200    while (!isTerminating())
     201    {
     202        Assert(m_workers[pWorker->getIndex()] == pWorker); /* paranoia */
     203
     204        /* Dequeue task if there is one: */
    171205        if (!m_tasks.isEmpty())
    172             pTask = m_tasks.dequeue();
    173     }
    174     Assert(isTerminating() || pTask || fOutdated);
    175 
    176     /* Unlock task locker: */
    177     m_taskLocker.unlock();
    178 
    179     /* Return task: */
    180     return pTask;
     206        {
     207            UITask *pTask = m_tasks.dequeue();
     208            if (pTask)
     209            {
     210                m_everythingLocker.unlock();
     211                return pTask;
     212            }
     213        }
     214
     215        /* If we timed out already, then quit the worker thread.  To prevent a
     216           race between enqueueTask and the queue removal of the thread from
     217           the workers vector, we remove it here already.  (This does not apply
     218           to the termination scenario.) */
     219        if (fIdleTimedOut)
     220        {
     221            m_workers[pWorker->getIndex()] = NULL;
     222            m_cWorkers--;
     223            break;
     224        }
     225
     226        /* Wait for a task or timeout.*/
     227        m_cIdleWorkers++;
     228        fIdleTimedOut = !m_taskCondition.wait(&m_everythingLocker, m_cMsIdleTimeout);
     229        m_cIdleWorkers--;
     230    }
     231
     232    m_everythingLocker.unlock();
     233
     234    return NULL;
    181235}
    182236
    … …  
    193247void UIThreadPool::sltHandleWorkerFinished(UIThreadWorker *pWorker)
    194248{
    195     /* Skip on termination: */
    196     if (isTerminating())
    197         return;
    198 
    199     /* Make sure that is one of our workers: */
    200     int iIndexOfWorker = m_workers.indexOf(pWorker);
    201     AssertReturnVoid(iIndexOfWorker != -1);
    202 
    203     /* Cleanup worker: */
    204     cleanupWorker(iIndexOfWorker);
    205 }
    206 
    207 void UIThreadPool::cleanupWorker(int iWorkerIndex)
    208 {
    209     /* Cleanup worker if any: */
    210     if (!m_workers[iWorkerIndex])
    211         return;
    212     m_workers[iWorkerIndex]->wait();
    213     delete m_workers[iWorkerIndex];
    214     m_workers[iWorkerIndex] = 0;
     249    /* Wait for the thread to finish completely, then delete the thread
     250       object.  We have already removed the thread from the workers vector.
     251       Note! We don't want to use 'this' here, in case it's invalid. */
     252    pWorker->wait();
     253    delete pWorker;
    215254}
    216255
    … …  
    238277    : m_pPool(pPool)
    239278    , m_iIndex(iIndex)
    240     , m_fBusy(true)
    241 {
    242 }
    243 
    244 bool UIThreadWorker::isBusy() const
    245 {
    246     /* Acquire busy-flag: */
    247     bool fBusy = false;
    248     {
    249         QMutexLocker lock(&m_busyLocker);
    250         fBusy = m_fBusy;
    251         Q_UNUSED(lock);
    252     }
    253     return fBusy;
    254 }
    255 
    256 void UIThreadWorker::setBusy(bool fBusy)
    257 {
    258     /* Assign busy-flag: */
    259     QMutexLocker lock(&m_busyLocker);
    260     m_fBusy = fBusy;
    261     Q_UNUSED(lock);
     279    , m_fNoFinishedSignal(false)
     280{
    262281}
    263282
    … …  
    269288    COMBase::InitializeCOM(false);
    270289
    271     /* Try to get task from thread-pool queue: */
     290    /* Try get a task from the pool queue. */
    272291    while (UITask *pTask = m_pPool->dequeueTask(this))
    273292    {
    … …  
    276295        if (!m_pPool->isTerminating())
    277296            pTask->start();
     297        /** @todo else: Just leak the task? */
    278298//        LogRelFlow(("UIThreadWorker #%d: Task processed!\n", m_iIndex));
    279299    }
    … …  
    282302    COMBase::CleanupCOM();
    283303
    284     /* Notify listener: */
    285     emit sigFinished(this);
     304    /* Queue a signal to for the pool to do thread cleanup, unless the pool is
     305       already terminating and doesn't need the signal. */
     306    if (!m_fNoFinishedSignal)
     307        emit sigFinished(this);
    286308
    287309//    LogRelFlow(("UIThreadWorker #%d: Finished!\n", m_iIndex));

  • trunk/src/VBox/Frontends/VirtualBox/src/globals/UIThreadPool.h

    r48270 r48908  
    11/** @file
    2  *
    3  * VBox frontends: Qt GUI ("VirtualBox"):
    4  * UIThreadPool class declaration
     2 * VBox Qt GUI - UIThreadPool and UITask class declaration.
    53 */
    64
    … …  
    1715 */
    1816
    19 #ifndef __UIThreadPool_h__
    20 #define __UIThreadPool_h__
     17#ifndef ___UIThreadPool_h___
     18#define ___UIThreadPool_h___
    2119
    2220/* Qt includes: */
    … …  
    4644
    4745    /* Constructor/destructor: */
    48     UIThreadPool(ulong uWorkerCount = 3, ulong uWorkerIdleTimeout = 5000);
     46    UIThreadPool(ulong cMaxWorkers = 3, ulong cMsWorkerIdleTimeout = 5000);
    4947    ~UIThreadPool();
    5048
    … …  
    5452    /* API: Termination stuff: */
    5553    bool isTerminating() const;
    56     void setTerminating(bool fTermintating);
     54    void setTerminating();
    5755
    5856protected:
    5957
    6058    /* Protected API: Worker-thread stuff: */
    61     UITask* dequeueTask(UIThreadWorker *pWorker);
     59    UITask *dequeueTask(UIThreadWorker *pWorker);
    6260
    6361private slots:
    … …  
    7068
    7169private:
     70    /** @name Worker thread related variables.
     71     * @{ */
     72    QVector<UIThreadWorker*> m_workers;
     73    /** Milliseconds  */
     74    const ulong m_cMsIdleTimeout;
     75    /** The number of worker threads.
     76     * @remarks We cannot use the vector size since it may contain NULL pointers. */
     77    int m_cWorkers;
     78    /** The number of idle worker threads. */
     79    int m_cIdleWorkers;
     80    /** Set by the destructor to indicate that all worker threads should
     81     *  terminate ASAP. */
     82    bool m_fTerminating;
     83    /** @} */
    7284
    73     /* Helper: Worker stuff: */
    74     void cleanupWorker(int iWorkerIndex);
     85    /** @name Task related variables
     86     * @{ */
     87    /** Queue (FIFO) of pending tasks. */
     88    QQueue<UITask*> m_tasks;
     89    /** Condition variable that gets signalled when queuing a new task and there are
     90     * idle worker threads around.
     91     *
     92     * Idle threads sits in dequeueTask waiting for this. Thus on thermination
     93     * setTerminating() will do a broadcast signal to wake up all workers (after
     94     * setting m_fTerminating of course). */
     95    QWaitCondition m_taskCondition;
     96    /** @} */
    7597
    76     /* Variables: Worker stuff: */
    77     QVector<UIThreadWorker*> m_workers;
    78     const ulong m_uIdleTimeout;
    7998
    80     /* Variables: Task stuff: */
    81     QQueue<UITask*> m_tasks;
    82     QMutex m_taskLocker;
    83     QWaitCondition m_taskCondition;
    84 
    85     /* Variables: Termination stuff: */
    86     bool m_fTerminating;
    87     mutable QMutex m_terminationLocker;
     99    /** This mutex is used with the m_taskCondition condition and protects m_tasks,
     100     *  m_fTerminating and m_workers. */
     101    mutable QMutex m_everythingLocker;
    88102
    89103    /* Friends: */
    … …  
    122136};
    123137
    124 #endif /* __UIThreadPool_h__ */
     138#endif /* !___UIThreadPool_h___ */
     139
Note: See TracChangeset for help on using the changeset viewer.

© 2024 Oracle Support Privacy / Do Not Sell My Info Terms of Use Trademark Policy Automated Access Etiquette