Changeset 27903 for trunk/psModules

Timestamp:

May 10, 2010, 12:47:31 PM (16 years ago)

Author:

Paul Price

Message:

Be more careful about locking. Despite protestations in the comments, locking *is* neccessary when going through the fitSets, because otherwise another thread can pull a fitSet out from under us (e.g., free it or set it to NULL while we're examining it).

File:

• trunk/psModules/src/objects/pmSourceFitSet.c (modified) (5 diffs)

trunk/psModules/src/objects/pmSourceFitSet.c

@@ -62 +62 @@
     // before pmSourceFitSetInit is called
     for (int i = 0; i < fitSets->n; i++) {
-        psAssert (fitSets->data[i] == NULL, "failure to init or clear fitSets?");
+        psAssert(fitSets->data[i] == NULL, "failure to init or clear fitSets?");
     }
     return true;
@@ -68 +68 @@
 
 void pmSourceFitSetDone (void) {
-    psFree (fitSets);
+    psFree(fitSets);
+    fitSets = NULL;
 }
 
@@ -113 +114 @@
 }
 
-pmSourceFitSetData *pmSourceFitSetDataSet (psArray *modelSet) {
-
-    pmSourceFitSetData *thisSet = NULL;
-
-    psAssert (fitSets, "pmSourceFitSetInit not called");
+pmSourceFitSetData *pmSourceFitSetDataSet (psArray *modelSet)
+{
+    psAssert(fitSets, "pmSourceFitSetInit not called");
 
     // find the fitSet used by this thread
     pthread_t id = pthread_self();
 
-    // is our ID is already on the stack, abort.
-    // we do not need to lock to do this....
+    // If our ID is already on the stack, abort.
+    // We do need to lock on this because someone might pull one of the fitSets out from under us
+    pthread_mutex_lock(&fitSetInitMutex);
     for (int i = 0; i < fitSets->n; i++) {
-        thisSet = fitSets->data[i];
+        pmSourceFitSetData *thisSet = fitSets->data[i];
         if (!thisSet) continue;
         if (thisSet->thread == id) {
-            break;
-        }
-        thisSet = NULL;
-    }
-    psAssert (thisSet == NULL, "pmSourceFitSetDataSet() called but previous entry not cleared");
-
-    thisSet = pmSourceFitSetDataAlloc(modelSet);
-
-    pthread_mutex_lock (&fitSetInitMutex);
-
-    // find an entry that is NULL and place it there
+            psAbort("pmSourceFitSetDataSet() called but previous entry not cleared");
+        }
+    }
+
+    // Find an open slot
     for (int i = 0; i < fitSets->n; i++) {
         if (fitSets->data[i]) continue;
-        fitSets->data[i] = thisSet;
-        pthread_mutex_unlock (&fitSetInitMutex);
+        pmSourceFitSetData *thisSet = fitSets->data[i] = pmSourceFitSetDataAlloc(modelSet);
+        pthread_mutex_unlock(&fitSetInitMutex);
         return thisSet;
     }
-    pthread_mutex_unlock (&fitSetInitMutex);
-    psAbort ("no empty slot for new pmSourceFitSetData");
+    pthread_mutex_unlock(&fitSetInitMutex);
+    psAbort("no empty slot for new pmSourceFitSetData");
     return NULL;
 }
@@ -152 +146 @@
 pmSourceFitSetData *pmSourceFitSetDataGet (void) {
 
-    psAssert (fitSets, "pmSourceFitSetInit not called");
-
-    // find the fitSet used by this thread
+    psAssert(fitSets, "pmSourceFitSetInit not called");
+
+    // Find the fitSet used by this thread
+    // We do need to lock on this because someone might pull one of the fitSets out from under us
     pthread_t id = pthread_self();
-
-    // can we find our fitSet?  we do not need to lock to do this....
-    pmSourceFitSetData *thisSet = NULL;
+    pthread_mutex_lock(&fitSetInitMutex);
     for (int i = 0; i < fitSets->n; i++) {
-        thisSet = fitSets->data[i];
+        pmSourceFitSetData *thisSet = fitSets->data[i];
         if (!thisSet) continue;
         if (thisSet->thread == id) {
-            break;
-        }
-        thisSet = NULL;
-    }
-    psAssert (thisSet != NULL, "pmSourceFitSetDataGet() called, but no entry found");
-
-    return thisSet;
-}
-
-void pmSourceFitSetDataClear (void) {
-
-    int i;
-
+            pthread_mutex_unlock(&fitSetInitMutex);
+            return thisSet;
+        }
+    }
+    psAbort("pmSourceFitSetDataGet() called, but no entry found");
+}
+
+void pmSourceFitSetDataClear (void)
+{
     psAssert (fitSets, "pmSourceFitSetInit not called");
 
-    // find the fitSet used by this thread
+    // Find the fitSet used by this thread
+    // We do need to lock on this because someone might pull one of the fitSets out from under us
     pthread_t id = pthread_self();
-
-    // can we find our fitSet?  we do not need to lock to do this....
-    pmSourceFitSetData *thisSet = NULL;
-    for (i = 0; i < fitSets->n; i++) {
-        thisSet = fitSets->data[i];
+    pthread_mutex_lock(&fitSetInitMutex);
+    for (int i = 0; i < fitSets->n; i++) {
+        pmSourceFitSetData *thisSet = fitSets->data[i];
         if (!thisSet) continue;
         if (thisSet->thread == id) {
-            break;
-        }
-        thisSet = NULL;
-    }
-    psAssert (thisSet != NULL, "pmSourceFitSetDataClear() called, but no entry found");
-
-    psFree (thisSet);
-    fitSets->data[i] = NULL;
+            psFree(thisSet);
+            fitSets->data[i] = NULL;
+            pthread_mutex_unlock(&fitSetInitMutex);
+            return;
+        }
+    }
+    psAbort("pmSourceFitSetDataClear() called, but no entry found");
+
     return;
 }
@@ -481 +469 @@
 
             // Convert i/j to image space:
-            // 0.5 PIX: the coordinate values must be in pixel coords, not index            
+            // 0.5 PIX: the coordinate values must be in pixel coords, not index
             coord->data.F32[0] = (psF32) (j + 0.5 + source->pixels->col0);
             coord->data.F32[1] = (psF32) (i + 0.5 + source->pixels->row0);