Clean up thread-local error stacks in all threads (#4852)

* Clean up error stacks from secondary threads

src/H5E.c

@@ -611,7 +611,7 @@ H5Eclear2(hid_t err_stack)
     } /* end else */
 
     /* Clear the error stack */
-    if (H5E__clear_stack(estack) < 0)
+    if (H5E__destroy_stack(estack) < 0)
         HGOTO_ERROR(H5E_ERROR, H5E_CANTSET, FAIL, "can't clear error stack");
 
 done:

src/H5Eint.c

@@ -646,7 +646,7 @@ H5E__get_current_stack(void)
     estack_copy->auto_data = current_stack->auto_data;
 
     /* Empty current error stack */
-    H5E__clear_stack(current_stack);
+    H5E__destroy_stack(current_stack);
 
     /* Set the return value */
     ret_value = estack_copy;
@@ -685,7 +685,7 @@ H5E__set_current_stack(H5E_stack_t *estack)
         HGOTO_ERROR(H5E_ERROR, H5E_CANTGET, FAIL, "can't get current error stack");
 
     /* Empty current error stack */
-    H5E__clear_stack(current_stack);
+    H5E__destroy_stack(current_stack);
 
     /* Copy new stack to current error stack */
     current_stack->nused = estack->nused;
@@ -715,7 +715,7 @@ H5E__close_stack(H5E_stack_t *estack, void H5_ATTR_UNUSED **request)
     assert(estack);
 
     /* Release the stack's error information */
-    H5E__clear_stack(estack);
+    H5E__destroy_stack(estack);
 
     /* Free the stack structure */
     estack = H5FL_FREE(H5E_stack_t, estack);
@@ -1708,16 +1708,20 @@ H5E_clear_stack(void)
 } /* end H5E_clear_stack() */
 
 /*-------------------------------------------------------------------------
- * Function:    H5E__clear_stack
+ * Function:    H5E__destroy_stack
  *
- * Purpose:     Clear the specified error stack
+ * Purpose:     Clear all internal state within an error stack, as a precursor to freeing it.
+ *
+ *              At present, this is nearly identical to H5E_clear_stack(),
+ *              but if additional resources are added to the error stack in the future,
+ *              they will only be released by this routine and not by H5E_clear_stack().
  *
  * Return:      SUCCEED/FAIL
  *
  *-------------------------------------------------------------------------
  */
 herr_t
-H5E__clear_stack(H5E_stack_t *estack)
+H5E__destroy_stack(H5E_stack_t *estack)
 {
     herr_t ret_value = SUCCEED; /* Return value */
 
@@ -1735,7 +1739,7 @@ H5E__clear_stack(H5E_stack_t *estack)
 
 done:
     FUNC_LEAVE_NOAPI(ret_value)
-} /* end H5E__clear_stack() */
+} /* end H5E__destroy_stack() */
 
 /*-------------------------------------------------------------------------
  * Function:    H5E__pop

src/H5Epkg.h

@@ -153,6 +153,6 @@ H5_DLL herr_t       H5E__get_auto(const H5E_stack_t *estack, H5E_auto_op_t *op,
 H5_DLL herr_t       H5E__set_auto(H5E_stack_t *estack, const H5E_auto_op_t *op, void *client_data);
 H5_DLL herr_t       H5E__pop(H5E_stack_t *err_stack, size_t count);
 H5_DLL herr_t       H5E__append_stack(H5E_stack_t *dst_estack, const H5E_stack_t *src_stack);
-H5_DLL herr_t       H5E__clear_stack(H5E_stack_t *estack);
+H5_DLL herr_t       H5E__destroy_stack(H5E_stack_t *estack);
 
 #endif /* H5Epkg_H */

src/H5TSint.c

@@ -524,10 +524,12 @@ H5TS__tinfo_destroy(void *_tinfo_node)
     FUNC_ENTER_PACKAGE_NAMECHECK_ONLY
 
     if (tinfo_node) {
-        /* Add thread info node to the free list */
         H5TS_mutex_lock(&H5TS_tinfo_mtx_s);
+        /* Add thread info node to the free list */
         tinfo_node->next       = H5TS_tinfo_next_free_s;
         H5TS_tinfo_next_free_s = tinfo_node;
+        /* Release resources held by error records in thread-local error stack */
+        H5E__destroy_stack(&tinfo_node->info.err_stack);
         H5TS_mutex_unlock(&H5TS_tinfo_mtx_s);
     }
 

test/CMakeLists.txt

@@ -355,6 +355,7 @@ set (ttsafe_SOURCES
     ${HDF5_TEST_SOURCE_DIR}/ttsafe_semaphore.c
     ${HDF5_TEST_SOURCE_DIR}/ttsafe_thread_id.c
     ${HDF5_TEST_SOURCE_DIR}/ttsafe_thread_pool.c
+    ${HDF5_TEST_SOURCE_DIR}/ttsafe_error_stacks.c
 )
 
 set (H5_EXPRESS_TESTS

test/Makefile.am

@@ -163,7 +163,7 @@ LDADD=libh5test.la $(LIBHDF5)
 ttsafe_SOURCES=ttsafe.c ttsafe_acreate.c ttsafe_atomic.c ttsafe_attr_vlen.c \
     ttsafe_cancel.c ttsafe_dcreate.c ttsafe_develop.c ttsafe_error.c \
     ttsafe_rwlock.c ttsafe_rec_rwlock.c ttsafe_semaphore.c \
-    ttsafe_thread_id.c ttsafe_thread_pool.c
+    ttsafe_thread_id.c ttsafe_thread_pool.c ttsafe_error_stacks.c
 cache_image_SOURCES=cache_image.c genall5.c
 mirror_vfd_SOURCES=mirror_vfd.c genall5.c
 

test/ttsafe.c

@@ -144,6 +144,8 @@ main(int argc, char *argv[])
 #ifdef H5_HAVE_THREADSAFE
     AddTest("thread_id", tts_thread_id, NULL, "thread IDs", NULL);
 
+    /* Error stack test must be done after thread_id test to not mess up expected IDs */
+    AddTest("error_stacks", tts_error_stacks, NULL, "error stack tests", NULL);
     AddTest("dcreate", tts_dcreate, cleanup_dcreate, "multi-dataset creation", NULL);
     AddTest("error", tts_error, cleanup_error, "per-thread error stacks", NULL);
 #ifdef H5_HAVE_PTHREAD_H

test/ttsafe.h

@@ -54,6 +54,7 @@ void tts_acreate(void);
 void tts_attr_vlen(void);
 void tts_thread_id(void);
 void tts_develop_api(void);
+void tts_error_stacks(void);
 
 /* Prototypes for the cleanup routines */
 void cleanup_dcreate(void);

test/ttsafe_error_stacks.c

@@ -0,0 +1,112 @@
+/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
+ * Copyright by The HDF Group.                                               *
+ * All rights reserved.                                                      *
+ *                                                                           *
+ * This file is part of HDF5.  The full HDF5 copyright notice, including     *
+ * terms governing use, modification, and redistribution, is contained in    *
+ * the COPYING file, which can be found at the root of the source code       *
+ * distribution tree, or in https://www.hdfgroup.org/licenses.               *
+ * If you do not have access to either file, you may request a copy from     *
+ * [email protected].                                                        *
+ * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */
+#include "ttsafe.h"
+
+#ifdef H5_HAVE_THREADSAFE
+
+#define ERR_CLS_NAME        "Custom error class"
+#define ERR_CLS_LIB_NAME    "example_lib"
+#define ERR_CLS_LIB_VERSION "0.1"
+
+#define ERR_MAJOR_MSG "Okay, Houston, we've had a problem here"
+#define ERR_MINOR_MSG "Oops!"
+
+H5TS_THREAD_RETURN_TYPE generate_hdf5_error(void *arg);
+H5TS_THREAD_RETURN_TYPE generate_user_error(void *arg);
+
+hid_t err_cls_id = H5I_INVALID_HID;
+
+/* Helper routine to generate an HDF5 library error */
+H5TS_THREAD_RETURN_TYPE
+generate_hdf5_error(void H5_ATTR_UNUSED *arg)
+{
+    H5TS_thread_ret_t ret_value = 0;
+    ssize_t           nobjs     = 0;
+
+    H5E_BEGIN_TRY
+    {
+        nobjs = H5Fget_obj_count(H5I_INVALID_HID, H5F_OBJ_ALL);
+    }
+    H5E_END_TRY
+
+    /* Expect call to fail */
+    VERIFY(nobjs, FAIL, "H5Fget_obj_count");
+
+    return ret_value;
+}
+
+/* Helper routine to generate a user-defined error */
+H5TS_THREAD_RETURN_TYPE
+generate_user_error(void H5_ATTR_UNUSED *arg)
+{
+    H5TS_thread_ret_t ret_value = 0;
+    hid_t             major     = H5I_INVALID_HID;
+    hid_t             minor     = H5I_INVALID_HID;
+    herr_t            status    = FAIL;
+
+    err_cls_id = H5Eregister_class(ERR_CLS_NAME, ERR_CLS_LIB_NAME, ERR_CLS_LIB_VERSION);
+    CHECK(err_cls_id, H5I_INVALID_HID, "H5Eregister_class");
+
+    major = H5Ecreate_msg(err_cls_id, H5E_MAJOR, ERR_MAJOR_MSG);
+    CHECK(major, H5I_INVALID_HID, "H5Ecreate_msg");
+
+    minor = H5Ecreate_msg(err_cls_id, H5E_MINOR, ERR_MINOR_MSG);
+    CHECK(minor, H5I_INVALID_HID, "H5Ecreate_msg");
+
+    status = H5Epush2(H5E_DEFAULT, __FILE__, __func__, __LINE__, err_cls_id, major, minor, "Hello, error\n");
+    CHECK(status, FAIL, "H5Epush2");
+
+    return ret_value;
+}
+
+/*
+**********************************************************************
+* tts_error_stacks
+*
+* Test that error stacks with user-defined error classes and messages
+* in secondary threads are properly cleaned up at library shutdown time.
+**********************************************************************
+*/
+void
+tts_error_stacks(void)
+{
+    H5TS_thread_t threads[2];
+    herr_t        status = FAIL;
+
+    /* Open library */
+    H5open();
+
+    status = H5TS_thread_create(&threads[0], generate_hdf5_error, NULL);
+    CHECK(status, FAIL, "H5TS_thread_create");
+
+    status = H5TS_thread_join(threads[0], NULL);
+    CHECK(status, FAIL, "H5TS_thread_join");
+
+    status = H5TS_thread_create(&threads[1], generate_user_error, NULL);
+    CHECK(status, FAIL, "H5TS_thread_create");
+
+    status = H5TS_thread_join(threads[1], NULL);
+    CHECK(status, FAIL, "H5TS_thread_join");
+
+    if (err_cls_id <= 0) {
+        TestErrPrintf("Failed to set up user error\n");
+        return;
+    }
+
+    status = H5Eunregister_class(err_cls_id);
+    CHECK(status, FAIL, "H5Eunregister_class");
+
+    /* Close library */
+    H5close();
+}
+
+#endif