Changeset 28405 for trunk/psphot/src/psphotBlendFit.c

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).

File:

• trunk/psphot/src/psphotBlendFit.c (modified) (7 diffs)

trunk/psphot/src/psphotBlendFit.c

@@ -15 +15 @@
     // loop over the available readouts
     for (int i = 0; i < num; i++) {
-        if (!psphotBlendFitReadout (config, view, filerule, i, recipe)) {
+        if (!psphotBlendFitReadout (config, view, filerule, i, recipe)) {
             psError (PSPHOT_ERR_CONFIG, false, "failed to fit sources (non-linear) for %s entry %d", filerule, i);
-            return false;
-        }
+            return false;
+        }
     }
     return true;
@@ -48 +48 @@
 
     if (!sources->n) {
-        psLogMsg ("psphot", PS_LOG_INFO, "no sources, skipping blend fit");
-        return true;
+        psLogMsg ("psphot", PS_LOG_INFO, "no sources, skipping blend fit");
+        return true;
     }
 
@@ -87 +87 @@
     sources = psArraySort (sources, pmSourceSortBySN);
     if (!sources->n) {
-        psLogMsg ("psphot", PS_LOG_INFO, "no sources, skipping blend");
-        return true;
+        psLogMsg ("psphot", PS_LOG_INFO, "no sources, skipping blend");
+        return true;
     }
 
@@ -103 +103 @@
         for (int j = 0; j < cells->n; j++) {
 
-            // allocate a job -- if threads are not defined, this just runs the job
-            psThreadJob *job = psThreadJobAlloc ("PSPHOT_BLEND_FIT");
-            psArray *newSources = psArrayAllocEmpty(16);
-
-            psArrayAdd(job->args, 1, readout);
-            psArrayAdd(job->args, 1, recipe);
-            psArrayAdd(job->args, 1, cells->data[j]); // sources
-            psArrayAdd(job->args, 1, psf);
-            psArrayAdd(job->args, 1, newSources); // return for new sources
-            psFree (newSources);
-
-            PS_ARRAY_ADD_SCALAR(job->args, 0, PS_TYPE_S32); // this is used as a return value for Nfit
-            PS_ARRAY_ADD_SCALAR(job->args, 0, PS_TYPE_S32); // this is used as a return value for Npsf
-            PS_ARRAY_ADD_SCALAR(job->args, 0, PS_TYPE_S32); // this is used as a return value for Next
-            PS_ARRAY_ADD_SCALAR(job->args, 0, PS_TYPE_S32); // this is used as a return value for Nfail
-
-            if (!psThreadJobAddPending(job)) {
-                psError(PS_ERR_UNKNOWN, false, "Unable to guess model.");
-                psFree (job);
-                return NULL;
-            }
-            psFree(job);
+            // allocate a job -- if threads are not defined, this just runs the job
+            psThreadJob *job = psThreadJobAlloc ("PSPHOT_BLEND_FIT");
+            psArray *newSources = psArrayAllocEmpty(16);
+
+            psArrayAdd(job->args, 1, readout);
+            psArrayAdd(job->args, 1, recipe);
+            psArrayAdd(job->args, 1, cells->data[j]); // sources
+            psArrayAdd(job->args, 1, psf);
+            psArrayAdd(job->args, 1, newSources); // return for new sources
+            psFree (newSources);
+
+            PS_ARRAY_ADD_SCALAR(job->args, 0, PS_TYPE_S32); // this is used as a return value for Nfit
+            PS_ARRAY_ADD_SCALAR(job->args, 0, PS_TYPE_S32); // this is used as a return value for Npsf
+            PS_ARRAY_ADD_SCALAR(job->args, 0, PS_TYPE_S32); // this is used as a return value for Next
+            PS_ARRAY_ADD_SCALAR(job->args, 0, PS_TYPE_S32); // this is used as a return value for Nfail
+
+            if (!psThreadJobAddPending(job)) {
+                psError(PS_ERR_UNKNOWN, false, "Unable to guess model.");
+                return NULL;
+            }
 
 # if (0)
@@ -152 +150 @@
         }
 
-        // wait for the threads to finish and manage results
-        if (!psThreadPoolWait (false)) {
-            psError(PS_ERR_UNKNOWN, false, "Unable to guess model.");
-            return NULL;
-        }
-
-        // we have only supplied one type of job, so we can assume the types here
-        psThreadJob *job = NULL;
-        while ((job = psThreadJobGetDone()) != NULL) {
-            if (job->args->n < 1) {
-                fprintf (stderr, "error with job\n");
-            } else {
-                psScalar *scalar = NULL;
-                scalar = job->args->data[5];
-                Nfit += scalar->data.S32;
-                scalar = job->args->data[6];
-                Npsf += scalar->data.S32;
-                scalar = job->args->data[7];
-                Next += scalar->data.S32;
-                scalar = job->args->data[8];
-                Nfail += scalar->data.S32;
-
-                // add these back onto sources
-                psArray *newSources = job->args->data[4];
-                for (int j = 0; j < newSources->n; j++) {
-                    psArrayAdd (sources, 16, newSources->data[j]);
-                }
-            }
-            psFree(job);
+        // wait for the threads to finish and manage results
+        if (!psThreadPoolWait (false)) {
+            psError(PS_ERR_UNKNOWN, false, "Unable to guess model.");
+            return NULL;
+        }
+
+        // we have only supplied one type of job, so we can assume the types here
+        psThreadJob *job = NULL;
+        while ((job = psThreadJobGetDone()) != NULL) {
+            if (job->args->n < 1) {
+                fprintf (stderr, "error with job\n");
+            } else {
+                psScalar *scalar = NULL;
+                scalar = job->args->data[5];
+                Nfit += scalar->data.S32;
+                scalar = job->args->data[6];
+                Npsf += scalar->data.S32;
+                scalar = job->args->data[7];
+                Next += scalar->data.S32;
+                scalar = job->args->data[8];
+                Nfail += scalar->data.S32;
+
+                // add these back onto sources
+                psArray *newSources = job->args->data[4];
+                for (int j = 0; j < newSources->n; j++) {
+                    psArrayAdd (sources, 16, newSources->data[j]);
+                }
+            }
+            psFree(job);
             }
     }
@@ -278 +276 @@
                 psTrace ("psphot", 5, "source at %7.1f, %7.1f is ext", source->peak->xf, source->peak->yf);
                 Next ++;
-                source->mode |= PM_SOURCE_MODE_NONLINEAR_FIT;
+                source->mode |= PM_SOURCE_MODE_NONLINEAR_FIT;
                 continue;
             }
@@ -286 +284 @@
                 psTrace ("psphot", 5, "source at %7.1f, %7.1f is psf", source->peak->xf, source->peak->yf);
                 Npsf ++;
-                source->mode |= PM_SOURCE_MODE_NONLINEAR_FIT;
+                source->mode |= PM_SOURCE_MODE_NONLINEAR_FIT;
                 continue;
             }