Bug fix for locator locking code

Author: David Berry

dat1HandleLock.c

@@ -62,7 +62,7 @@
 *            otherwise (in which case the current thread does not have a lock,
 *            but some other threads may have read locks). If "recurs" is
 *            non-zero, +1 is returned only if the supplied Handle and all
-*            child handles are by the current thread. Otherwise, the
+*            child handles are locked by the current thread. Otherwise, the
 *            returned value relates to the first handle (either the supplied
 *            or a child handle) that was no locked by the current thread.
 *
@@ -265,7 +265,7 @@ Handle *dat1HandleLock( Handle *handle, int oper, int recurs, int rdonly,
             if( child ) {
 
 /* Get the lock status of the child. */
-               (void) dat1HandleLock( child, -2, 1, rdonly, &child_result,
+               (void) dat1HandleLock( child, -1, 1, rdonly, &child_result,
                                       status );
 
 /* If it's 2, we can set the final result and exit immediately. */
@@ -394,7 +394,7 @@ Handle *dat1HandleLock( Handle *handle, int oper, int recurs, int rdonly,
                child = handle->children[ichild];
                if( child ) {
                   error_handle = dat1HandleLock( child, -2, 1, rdonly,
-                                                 &child_result, status );
+                                                 result, status );
                   if( error_handle ) break;
                }
             }
@@ -455,7 +455,7 @@ Handle *dat1HandleLock( Handle *handle, int oper, int recurs, int rdonly,
                child = handle->children[ichild];
                if( child ) {
                   error_handle = dat1HandleLock( child, -3, 1, 0,
-                                                 &child_result, status );
+                                                 result, status );
                   if( error_handle ) break;
                }
             }

datLock.c

@@ -21,11 +21,10 @@
 *     recurs = int (Given)
 *        If the supplied object is locked successfully, and "recurs" is
 *        non-zero, then an attempt is made to lock any component objects
-*        contained within the supplied object. No error is reported if
-*        any components cannot be locked due to being locked already by
-*        a different thread - such components are left unchanged. This
-*        operation is recursive - any children of the child components
-*        are also locked, etc.
+*        contained within the supplied object. An error is reported if
+*        any components cannot be locked due to them being locked already
+*        by a different thread. This operation is recursive - any children
+*        of the child components are also locked, etc.
 *     readonly = int (Given)
 *        If non-zero, the object (and child objects if "recurs" is non-zero)
 *        is locked for read-only access. Otherwise it is locked for
@@ -72,9 +71,9 @@
 
 *  Notes:
 *     - An error will be reported if the supplied object is currently
-*     locked by another thread, but no error is reported if any component
-*     objects contained within the supplied object are locked by other
-*     threads (such objects are left unchanged).
+*     locked by another thread. If "recurs" is non-zero, an error is
+*     also reported if any component objects contained within the
+*     supplied object are locked by other threads.
 *     - The majority of HDS functions will report an error if the object
 *     supplied to the function has not been locked for use by the calling
 *     thread. The exceptions are the functions that manage these locks -

datUnlock.c

@@ -21,17 +21,15 @@
 *     recurs = int (Given)
 *        If the supplied object is unlocked successfully, and "recurs" is
 *        non-zero, then an attempt is made to unlock any component objects
-*        contained within the supplied object. No error is reported if
-*        any components cannot be unlocked due to being locked already by
-*        a different thread - such components are left unchanged. This
-*        operation is recursive - any children of the child components
-*        are also unlocked, etc.
+*        contained within the supplied object.
 *     status = int* (Given and Returned)
 *        Pointer to global status.
 
 *  Description:
 *     This function removes a lock on the supplied HDS object. An error
-*     is reported if the current thread has no lock to be removed. See
+*     is reported if the object is not locked by the current thread. If
+*     "recurs" is non-zero, an error will be reported if any child component
+*     within the supplied object is not locked by the current thread. See
 *     datLock.
 *
 *     The object must be locked again, using datLock, before it can be

hdsTest.c

@@ -68,6 +68,7 @@
 #include <stdio.h>
 #include <inttypes.h>
 #include <string.h>
+#include <pthread.h>
 
 static void traceme (const HDSLoc * loc, const char * expected, int explev,
                      int *status);
@@ -81,6 +82,8 @@ static void testThreadSafety( const char *path, int *status );
 static void *test1ThreadSafety( void *data );
 static void *test2ThreadSafety( void *data );
 static void *test3ThreadSafety( void *data );
+void showloc( HDSLoc *loc, const char *title, int ind );
+void showhan( Handle *h, int ind );
 
 typedef struct threadData {
    HDSLoc *loc;
@@ -90,6 +93,9 @@ typedef struct threadData {
    int status;
 } threadData;
 
+pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
+
+
 int main (void) {
 
   /*  Local Variables: */
@@ -1161,7 +1167,9 @@ static void testThreadSafety( const char *path, int *status ) {
    }
 
 /* Attempt to access the two top-level objects in two separate threads.
-   Each thread attempt to lock the object read/write, but only one can win.
+   Each thread attempt to lock the object read/write, but only one can
+   win (assuming that the second thread starts up before the first thread
+   has finished and unlocked the object).
    The other should report an error. */
    if( *status == SAI__OK ) {
       threaddata1.id = 1;
@@ -1197,9 +1205,6 @@ static void testThreadSafety( const char *path, int *status ) {
                   threaddata1.failed + threaddata2.failed );
       }
 
-/* Unlock them now that the test has completed. */
-      datUnlock( loc1, 1, status );
-      datUnlock( loc1b, 1, status );
    }
 
 /* Attempt to access the two top-level objects in two separate threads.
@@ -1236,10 +1241,6 @@ static void testThreadSafety( const char *path, int *status ) {
                   "- expected 0 to fail.",  status,
                   threaddata1.failed + threaddata2.failed );
       }
-
-/* Unlock them now that the test has completed. */
-      datUnlock( loc1, 1, status );
-      datUnlock( loc1b, 1, status );
    }
 
 /* Lock both top level locators for read-only use by the current thread. The
@@ -1410,23 +1411,42 @@ void *test2ThreadSafety( void *data ) {
    HDSLoc *loc2 = NULL;
    int status = SAI__OK;
    int expect = tdata->rdonly ? 3 : 1;
+   int i, ii;
 
    datLock( loc1, 1, tdata->rdonly, &status );
    if( status == DAT__THREAD ) {
       emsAnnul( &status );
       tdata->failed = 1;
-   } else {
+   } else if( status == SAI__OK ){
+
       tdata->failed = 0;
       datFind( loc1, "Records", &loc2, &status );
 
       /* Check the component locator is locked by the current thread. */
-      if( datLocked( loc2, 1, &status ) != expect && status == SAI__OK ) {
+      ii = datLocked( loc2, 1, &status );
+      if( ii != expect && status == SAI__OK ) {
          status = DAT__FATAL;
          emsRepf("", "testThreadSafety error B1: loc2 is not locked by "
-                 "current thread.", &status );
+                 "current thread (%d %d). ", &status, ii, expect );
 
       }
       datAnnul( &loc2, &status );
+
+      /* Do something time consuming to make it likely that this thread
+         will not have finished (and so unlocked the object), before the other
+         thread starts. */
+      for( i = 0; i < 1000000; i++ ) {
+         ii = datLocked( loc1, 1, &status );
+         if( ii != expect && status == SAI__OK ) {
+            status = SAI__ERROR;
+            emsRepf( " ", "test2ThreadSafety: Unexpected lock status %d - "
+                     "expected %d", &status, ii, expect );
+            break;
+         }
+      }
+
+      datUnlock( loc1, 1, &status );
+
    }
 
    tdata->status = status;
@@ -1481,3 +1501,36 @@ void *test3ThreadSafety( void *data ) {
 
 
 
+/* Display information about the locs on a locator. */
+void showloc( HDSLoc *loc, const char *title, int indent ) {
+   int i;
+   pthread_mutex_lock(&mutex);
+   printf("\n");
+   for( i = 0; i < indent; i++ ) printf(" ");
+   printf( "%s\n", title );
+   for( i = 0; i < indent; i++ ) printf(" ");
+   for( i = 0; i < strlen(title); i++ ) printf("-");
+   printf("\n");
+   showhan( loc->handle, indent );
+   pthread_mutex_unlock(&mutex);
+}
+
+void showhan( Handle *h, int indent ) {
+   if( !h ) return;
+
+   int i;
+   for( i = 0; i < indent; i++ ) printf(" ");
+   printf("'%s' ", h->name ? h->name : " " );
+   if( h->nwrite_lock ) printf("w:%zu ", h->write_locker );
+   for( i = 0; i < h->nread_lock; i++ ) {
+      printf("r:%zu ", h->read_lockers[ i ] );
+   }
+   printf("\n");
+
+   for( i = 0; i < h->nchild; i++ ) {
+      showhan( h->children[i], indent + 3 );
+   }
+}
+
+
+