Changeset 28405 for trunk/psLib

Timestamp:

Jun 18, 2010, 2:25:38 PM (16 years ago)

Author:

Paul Price

Message:

Discovered a race condition in the threading process. The thread process was: psThreadJobAlloc, populate job with arguments, psThreadJobAddPending, psFree the job. This is not thread-safe, because we're decrementing the job's reference counter while the reference count is also being fiddled within the thread system (psThread.c; by pulling the job off the 'pending' queue and putting it on the 'done' queue). The reference count fiddling within the thread system was protected by a mutex, but the external free was not. Therefore, I'm pulling the free into the thread system so it can be protected (without the user having to worry about locks to do the simple case). This means that the user should no longer touch the job once he hands it over to the thread system (unless it's protected by calls to psThreadLock/psThreadUnlock, or it's on the other side of psThreadPoolWait from all the adds so that the threads aren't doing anything).

Location:

trunk/psLib/src

Files:

• imageops/psImageConvolve.c (modified) (7 diffs)
• imageops/psImageCovariance.c (modified) (2 diffs)
• sys/psThread.c (modified) (4 diffs)
• sys/psThread.h (modified) (3 diffs)

trunk/psLib/src/imageops/psImageConvolve.c

@@ -864 +864 @@
             PS_ARRAY_ADD_SCALAR(job->args, stop, PS_TYPE_S32);
             if (!psThreadJobAddPending(job)) {
-                psFree(job);
                 psFree(gaussNorm);
                 psFree(out);
                 return NULL;
             }
-            psFree(job);
         }
         if (!psThreadPoolWait(true)) {
@@ -1213 +1211 @@
                   if (!psThreadJobAddPending(job)) {
                       psError(PS_ERR_UNKNOWN, false, "Unable to smooth image");
-                      psFree(job);
                       psFree(calculation);
                       psFree(calcMask);
@@ -1219 +1216 @@
                       return false;
                   }
-                  psFree(job);
               }
               // wait here for the threaded jobs to finish (NOP if threading is not active)
@@ -1261 +1257 @@
                   if (!psThreadJobAddPending(job)) {
                       psError(PS_ERR_UNKNOWN, false, "Unable to smooth image");
-                      psFree(job);
                       psFree(calculation);
                       psFree(calcMask);
@@ -1267 +1262 @@
                       return false;
                   }
-                  psFree(job);
               }
 
@@ -1580 +1574 @@
             PS_ARRAY_ADD_SCALAR(job->args, 0x00, PS_TYPE_U8); // specify rows
             if (!psThreadJobAddPending(job)) {
-                psFree(job);
                 psFree(conv);
                 psFree(out);
                 return NULL;
             }
-            psFree(job);
         }
         if (!psThreadPoolWait(true)) {
@@ -1617 +1609 @@
             PS_ARRAY_ADD_SCALAR(job->args, 0xff, PS_TYPE_U8); // specify cols
             if (!psThreadJobAddPending(job)) {
-                psFree(job);
                 psFree(conv);
                 psFree(out);
                 return NULL;
             }
-            psFree(job);
         }
         if (!psThreadPoolWait(true)) {

trunk/psLib/src/imageops/psImageCovariance.c

@@ -169 +169 @@
                     return NULL;
                 }
-                psFree(job);
             } else {
                 out->kernel[y][x] = imageCovarianceCalculate(covar, kernel, x, y);
@@ -334 +333 @@
                     return NULL;
                 }
-                psFree(job);
             } else {
                 out->kernel[y][x] = imageCovarianceBin(covar, bin, binVal, x, y);

trunk/psLib/src/sys/psThread.c

@@ -99 +99 @@
 bool psThreadJobAddPending(psThreadJob *job)
 {
-    PS_ASSERT_THREAD_JOB_NON_NULL(job, false);
+    if (!job) {
+        // Asking for a no-op
+        return true;
+    }
 
     psThreadTask *task = psHashLookup(tasks, job->type); // Task to execute job
@@ -114 +117 @@
         }
         psListAdd(done, PS_LIST_TAIL, job);
-
+        psFree(job);
         return task->function(job);
     }
@@ -123 +126 @@
     }
     psListAdd(pending, PS_LIST_TAIL, job);
+    psFree(job);
     psThreadUnlock();
 
@@ -279 +283 @@
     for (int i = 0; i < nThreads; i++) {
         psThread *thread = pool->data[i] = psThreadAlloc(); // Thread for pool
-        if (!pthread_create(&threads[i], NULL, psThreadLauncher, thread)) {
+        if (pthread_create(&threads[i], NULL, psThreadLauncher, thread)) {
             psAbort("Unable to create thread");
         }

trunk/psLib/src/sys/psThread.h

@@ -73 +73 @@
 
 /// Add a pending job to the queue
+///
+/// This function swallows the provided job, so that the user no longer owns it.  This is because freeing the
+/// job is not thread-safe (its reference count is being changed within the threads) so we handle it ourselves
+/// and absolve the user from all responsibility.  If the user stores the job, he should only access it while
+/// threads are processing in code protected by psThreadLock/psThreadUnlock.
 bool psThreadJobAddPending(psThreadJob *job);
 
 /// Get a job off the queue of pending jobs
+///
+/// This function is not thread-safe.  Protect with psThreadLock/psThreadUnlock if threads are running.
 psThreadJob *psThreadJobGetPending(void);
 
 /// Get a job off the queue of done jobs
+///
+/// This function is not thread-safe.  Protect with psThreadLock/psThreadUnlock if threads are running.
 psThreadJob *psThreadJobGetDone(void);
 
@@ -115 +124 @@
 bool psThreadPoolFinalize(void);
 
-
+#if 0
 /// Add thread-specific data
 ///
@@ -134 +143 @@
 bool psThreadDataRemove(const char *name // Name of data
     );
-
+#endif
 
 /// @}