From a2eca3b19d87febc087b53ebbd5a5ca327d44bb2 Mon Sep 17 00:00:00 2001 From: Chandra Penke Date: Thu, 20 Jan 2011 04:45:27 -0200 Subject: [PATCH] Fix for Race condition can hang miniserver thread. Add 'requiredThreads' field to the ThreadPool structure, to avoid a race condition when waiting for a new thread to be created. The race condition occurs when a thread is destroyed while the master thread is waiting for a new thread to be created. Thanks to Chuck Thomason for pointing the problem. Summary: Race condition can hang miniserver thread - ID: 3158591 Details: Hello, I have found a race condition in the thread pool handling of libupnp-1.6.6 that periodically results in the miniserver thread getting blocked infinitely. In my setup, I have the miniserver thread pool configured with 1 job per thread, 2 threads minimum, and 50 threads maximum. Just before the lockup occurs, the miniserver thread pool contains 2 threads: one worker thread hanging around from a previous HTTP request job (let's call that thread "old_worker") and the miniserver thread itself. A new HTTP request comes in. Accordingly, the miniserver enters schedule_request_job() and then ThreadPoolAdd(). In ThreadPoolAdd(), the job gets added to the medium-priority queue, and AddWorker() is called. In AddWorker(), jobs = 1 and threads = 1, so CreateWorker gets called. When we enter CreateWorker(), tp->totalThreads is 2, so currentThreads is 3. The function creates a new thread and then blocks on tp->start_and_shutdown. The miniserver thread expects the newly created thread to increment tp->totalThreads and then signal the condition variable to wake up the miniserver thread and let it proceed. The newly created thread starts in the WorkerThread() function. It increments tp->totalThreads to 3, does a broadcast on the start_and_shutdown condition, and starts running its job. However, before the miniserver thread wakes up, "old_worker" times out. It sees that there are no jobs in any queue and that the total number of threads (3) is more than the minimum (2). As a result, it reduces tp->totalThreads to 2 and dies. Now the miniserver thread finally wakes up. It checks tp->totalThreads and sees that its value is 2, so it blocks on tp->start_and_shutdown again. It has now "missed" seeing tp->totalThreads get incremented to 3 and will never be unblocked again. When this issue does occur for a server device, the miniserver port remains open, but becomes unresponsive since the miniserver thread is stuck. SSDP alive messages keep getting sent out, as they are handled by a separate thread. Reproducing the issue is difficult due to the timing coincidence involved, but in my environment I am presently seeing it at least once a day. I figured out the sequence described above through addition of my own debug logs. The relevant code involved in this bug has not changed substantially in libupnp-1.6.10, though I am planning to test against 1.6.10 as well in the near future. Do you have any input for an elegant fix for this issue? Thanks, Chuck Thomason (cherry picked from commit c4e9757bcf26c48791cc9c4e4f6a355e1faf49e5) --- ChangeLog | 73 +++++++++++++++++++++++++++++++++++++ threadutil/inc/ThreadPool.h | 2 + threadutil/src/ThreadPool.c | 27 +++++++++++--- 3 files changed, 96 insertions(+), 6 deletions(-) diff --git a/ChangeLog b/ChangeLog index ae265ca..1445901 100644 --- a/ChangeLog +++ b/ChangeLog @@ -255,6 +255,79 @@ Version 1.8.0 Version 1.6.11 ******************************************************************************* +2011-01-20 Chandra Penke + Fix for Race condition can hang miniserver thread. + + Add 'requiredThreads' field to the ThreadPool structure, to avoid + a race condition when waiting for a new thread to be created. The + race condition occurs when a thread is destroyed while the master + thread is waiting for a new thread to be created. + + Thanks to Chuck Thomason for pointing the problem. + + Summary: Race condition can hang miniserver thread - ID: 3158591 + + Details: + Hello, + + I have found a race condition in the thread pool handling of + libupnp-1.6.6 that periodically results in the miniserver thread + getting blocked infinitely. + + In my setup, I have the miniserver thread pool configured with 1 + job per thread, 2 threads minimum, and 50 threads maximum. + + Just before the lockup occurs, the miniserver thread pool contains + 2 threads: one worker thread hanging around from a previous HTTP + request job (let's call that thread "old_worker") and the + miniserver thread itself. + + A new HTTP request comes in. Accordingly, the miniserver enters + schedule_request_job() and then ThreadPoolAdd(). In + ThreadPoolAdd(), the job gets added to the medium-priority queue, + and AddWorker() is called. In AddWorker(), jobs = 1 and threads = + 1, so CreateWorker gets called. + + When we enter CreateWorker(), tp->totalThreads is 2, so + currentThreads is 3. The function creates a new thread and then + blocks on tp->start_and_shutdown. The miniserver thread expects + the newly created thread to increment tp->totalThreads and then + signal the condition variable to wake up the miniserver thread and + let it proceed. + + The newly created thread starts in the WorkerThread() function. It + increments tp->totalThreads to 3, does a broadcast on the + start_and_shutdown condition, and starts running its job. However, + before the miniserver thread wakes up, "old_worker" times out. It + sees that there are no jobs in any queue and that the total number + of threads (3) is more than the minimum (2). As a result, it + reduces tp->totalThreads to 2 and dies. + + Now the miniserver thread finally wakes up. It checks + tp->totalThreads and sees that its value is 2, so it blocks on + tp->start_and_shutdown again. It has now "missed" seeing + tp->totalThreads get incremented to 3 and will never be unblocked + again. + + When this issue does occur for a server device, the miniserver + port remains open, but becomes unresponsive since the miniserver + thread is stuck. SSDP alive messages keep getting sent out, as + they are handled by a separate thread. Reproducing the issue is + difficult due to the timing coincidence involved, but in my + environment I am presently seeing it at least once a day. I + figured out the sequence described above through addition of my + own debug logs. + + The relevant code involved in this bug has not changed + substantially in libupnp-1.6.10, though I am planning to test + against 1.6.10 as well in the near future. + + Do you have any input for an elegant fix for this issue? + + Thanks, + + Chuck Thomason + 2011-01-16 Marcelo Roberto Jimenez Define _FILE_OFFSET_BITS, _LARGEFILE_SOURCE and _LARGE_FILE_SOURCE in diff --git a/threadutil/inc/ThreadPool.h b/threadutil/inc/ThreadPool.h index 43e0a46..b98a150 100644 --- a/threadutil/inc/ThreadPool.h +++ b/threadutil/inc/ThreadPool.h @@ -222,6 +222,8 @@ typedef struct THREADPOOL int shutdown; /*! total number of threads */ int totalThreads; + /*! flag that's set when waiting for a new worker thread to start */ + int pendingWorkerThreadStart; /*! number of threads that are currently executing jobs */ int busyThreads; /*! number of persistent threads */ diff --git a/threadutil/src/ThreadPool.c b/threadutil/src/ThreadPool.c index 81b7595..35b568c 100644 --- a/threadutil/src/ThreadPool.c +++ b/threadutil/src/ThreadPool.c @@ -97,7 +97,8 @@ static void StatsInit( stats->workerThreads = 0; stats->idleThreads = 0; stats->persistentThreads = 0; - stats->maxThreads = 0; stats->totalThreads = 0; + stats->maxThreads = 0; + stats->totalThreads = 0; } /*! @@ -459,6 +460,7 @@ static void *WorkerThread( /* Increment total thread count */ ithread_mutex_lock(&tp->mutex); tp->totalThreads++; + tp->pendingWorkerThreadStart = 0; ithread_cond_broadcast(&tp->start_and_shutdown); ithread_mutex_unlock(&tp->mutex); @@ -602,6 +604,9 @@ static ThreadPoolJob *CreateThreadPoolJob( * \brief Creates a worker thread, if the thread pool does not already have * max threads. * + * \remark The ThreadPool object mutex must be locked prior to calling this + * function. + * * \internal * * \return @@ -610,16 +615,20 @@ static ThreadPoolJob *CreateThreadPoolJob( * \li \c EAGAIN if system can not create thread. */ static int CreateWorker( - /*! . */ + /*! A pointer to the ThreadPool object. */ ThreadPool *tp) { ithread_t temp; int rc = 0; - int currentThreads = tp->totalThreads + 1; ithread_attr_t attr; + /* if a new worker is the process of starting, wait until it fully starts */ + while (tp->pendingWorkerThreadStart) { + ithread_cond_wait(&tp->start_and_shutdown, &tp->mutex); + } + if (tp->attr.maxThreads != INFINITE_THREADS && - currentThreads > tp->attr.maxThreads) { + tp->totalThreads + 1 > tp->attr.maxThreads) { return EMAXTHREADS; } ithread_attr_init(&attr); @@ -628,7 +637,9 @@ static int CreateWorker( ithread_attr_destroy(&attr); if (rc == 0) { rc = ithread_detach(temp); - while (tp->totalThreads < currentThreads) { + tp->pendingWorkerThreadStart = 1; + /* wait until the new worker thread starts */ + while (tp->pendingWorkerThreadStart) { ithread_cond_wait(&tp->start_and_shutdown, &tp->mutex); } } @@ -643,10 +654,13 @@ static int CreateWorker( * \brief Determines whether or not a thread should be added based on the * jobsPerThread ratio. Adds a thread if appropriate. * + * \remark The ThreadPool object mutex must be locked prior to calling this + * function. + * * \internal */ static void AddWorker( - /*! . */ + /*! A pointer to the ThreadPool object. */ ThreadPool *tp) { long jobs = 0; @@ -709,6 +723,7 @@ int ThreadPoolInit(ThreadPool *tp, ThreadPoolAttr *attr) tp->totalThreads = 0; tp->busyThreads = 0; tp->persistentThreads = 0; + tp->pendingWorkerThreadStart = 0; for (i = 0; i < tp->attr.minThreads; ++i) { retCode = CreateWorker(tp); if (retCode) {