From e2da35371b59d12e81dfd7d83b38b0dc0d3310d0 Mon Sep 17 00:00:00 2001 From: Matt L <124107509+mattjala@users.noreply.github.com> Date: Tue, 1 Oct 2024 10:53:53 -0500 Subject: [PATCH] Clean up thread-local error stacks in all threads (#4852) * Clean up error stacks from secondary threads --- src/H5E.c | 2 +- src/H5Eint.c | 18 +++--- src/H5Epkg.h | 2 +- src/H5TSint.c | 4 +- test/CMakeLists.txt | 1 + test/Makefile.am | 2 +- test/ttsafe.c | 2 + test/ttsafe.h | 1 + test/ttsafe_error_stacks.c | 112 +++++++++++++++++++++++++++++++++++++ 9 files changed, 133 insertions(+), 11 deletions(-) create mode 100644 test/ttsafe_error_stacks.c diff --git a/src/H5E.c b/src/H5E.c index f1b865e7c17..a037ffa595e 100644 --- a/src/H5E.c +++ b/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: diff --git a/src/H5Eint.c b/src/H5Eint.c index 8db9d5fdff7..333bc202c0b 100644 --- a/src/H5Eint.c +++ b/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 diff --git a/src/H5Epkg.h b/src/H5Epkg.h index b758cbe4ec9..2eda2b24e2d 100644 --- a/src/H5Epkg.h +++ b/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 */ diff --git a/src/H5TSint.c b/src/H5TSint.c index 322b7fe98c0..c7600eb1fda 100644 --- a/src/H5TSint.c +++ b/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); } diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 02a9891d512..8c9b3073b9d 100644 --- a/test/CMakeLists.txt +++ b/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 diff --git a/test/Makefile.am b/test/Makefile.am index 061bfcf0df5..09aae88a361 100644 --- a/test/Makefile.am +++ b/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 diff --git a/test/ttsafe.c b/test/ttsafe.c index 2f5f26db331..068f0d40e40 100644 --- a/test/ttsafe.c +++ b/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 diff --git a/test/ttsafe.h b/test/ttsafe.h index 7544b3ff32d..8a2200cd9a7 100644 --- a/test/ttsafe.h +++ b/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); diff --git a/test/ttsafe_error_stacks.c b/test/ttsafe_error_stacks.c new file mode 100644 index 00000000000..40fd2453bdd --- /dev/null +++ b/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 * + * help@hdfgroup.org. * + * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */ +#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 \ No newline at end of file