From 39d0bb910233d49e802b46c4135d7a8787c9cd78 Mon Sep 17 00:00:00 2001 From: Scot Breitenfeld Date: Thu, 16 Jul 2026 00:07:29 -0500 Subject: [PATCH] Restore FUNC_ENTER_API_NOINIT_NOLOCK for H5Zconfig_get_* functions Revert fad067a1c18, which replaced these with the regular locking FUNC_ENTER_API_NOINIT on the premise that "the API lock is already recursive." That holds for HDF5_ENABLE_THREADSAFE (a genuinely recursive mutex) but not for HDF5_ENABLE_CONCURRENCY, whose H5_API_LOCK uses a per-call dlftt (disable-locking-for-this-thread) counter that each FUNC_ENTER_API_NOINIT call site initializes fresh rather than a mutex that recognizes same-thread reentry. Since H5Zconfig_get_int and friends are called from inside H5Z_set_config_func_t callbacks that run under H5Pappend_filter's already-held API lock, the second lock acquisition self-deadlocks under concurrency builds. Reproduced directly: built locally with -DHDF5_ENABLE_CONCURRENCY=ON (matching CI's Concurrency Workflows config) and confirmed tfilter2 hangs indefinitely on the first filter that invokes set_config with a real config string; after this revert it completes in the same run. This is what was actually causing the H5TEST-tfilter2 and H5REPACK_UD-plugin_test_cfg timeouts under CI's Concurrency Workflows. --- src/H5Zconfig.c | 34 +++++++++++++++++----------------- src/H5private.h | 31 +++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 17 deletions(-) diff --git a/src/H5Zconfig.c b/src/H5Zconfig.c index 8fa9fc11604..cceb3f50e49 100644 --- a/src/H5Zconfig.c +++ b/src/H5Zconfig.c @@ -492,16 +492,16 @@ H5Zconfig_has_key(const char *params, const char *key) toml_datum_t d; htri_t ret_value = FAIL; - /* The API lock is recursive, so this is safe to call from inside an - * H5Z_set_config_func_t callback that is already running under the API - * lock held by H5Pappend_filter. */ - FUNC_ENTER_API_NOINIT + /* No API lock: this is a pure parser over caller-provided buffers and + * may be called from inside an H5Z_set_config_func_t callback that is + * already running under the API lock held by H5Pappend_filter. */ + FUNC_ENTER_API_NOINIT_NOLOCK ret_value = H5Z__config_get_datum(params, key, &tr, &d); if (ret_value > 0) toml_free(tr); - FUNC_LEAVE_API_NOINIT(ret_value) + FUNC_LEAVE_API_NOINIT_NOLOCK(ret_value) } /*------------------------------------------------------------------------- @@ -578,12 +578,12 @@ H5Zconfig_get_int(const char *params, const char *key, int64_t *out) { htri_t ret_value = FAIL; - /* See comment on H5Zconfig_has_key about recursive re-entry. */ - FUNC_ENTER_API_NOINIT + /* No API lock: see comment on H5Zconfig_has_key. */ + FUNC_ENTER_API_NOINIT_NOLOCK ret_value = H5Z__config_get_int(params, key, out); - FUNC_LEAVE_API_NOINIT(ret_value) + FUNC_LEAVE_API_NOINIT_NOLOCK(ret_value) } /*------------------------------------------------------------------------- @@ -606,8 +606,8 @@ H5Zconfig_get_double(const char *params, const char *key, double *out) htri_t found; htri_t ret_value = FAIL; - /* See comment on H5Zconfig_has_key about recursive re-entry. */ - FUNC_ENTER_API_NOINIT + /* No API lock: see comment on H5Zconfig_has_key. */ + FUNC_ENTER_API_NOINIT_NOLOCK if (!out) HGOTO_ERROR(H5E_ARGS, H5E_BADVALUE, FAIL, "out must not be NULL"); @@ -628,7 +628,7 @@ H5Zconfig_get_double(const char *params, const char *key, double *out) done: if (tr_valid) toml_free(tr); - FUNC_LEAVE_API_NOINIT(ret_value) + FUNC_LEAVE_API_NOINIT_NOLOCK(ret_value) } /*------------------------------------------------------------------------- @@ -650,8 +650,8 @@ H5Zconfig_get_bool(const char *params, const char *key, bool *out) htri_t found; htri_t ret_value = FAIL; - /* See comment on H5Zconfig_has_key about recursive re-entry. */ - FUNC_ENTER_API_NOINIT + /* No API lock: see comment on H5Zconfig_has_key. */ + FUNC_ENTER_API_NOINIT_NOLOCK if (!out) HGOTO_ERROR(H5E_ARGS, H5E_BADVALUE, FAIL, "out must not be NULL"); @@ -669,7 +669,7 @@ H5Zconfig_get_bool(const char *params, const char *key, bool *out) done: if (tr_valid) toml_free(tr); - FUNC_LEAVE_API_NOINIT(ret_value) + FUNC_LEAVE_API_NOINIT_NOLOCK(ret_value) } /*------------------------------------------------------------------------- @@ -763,10 +763,10 @@ H5Zconfig_get_str(const char *params, const char *key, char *buf, size_t *buf_si { htri_t ret_value = FAIL; - /* See comment on H5Zconfig_has_key about recursive re-entry. */ - FUNC_ENTER_API_NOINIT + /* No API lock: see comment on H5Zconfig_has_key. */ + FUNC_ENTER_API_NOINIT_NOLOCK ret_value = H5Z__config_get_str(params, key, buf, buf_size); - FUNC_LEAVE_API_NOINIT(ret_value) + FUNC_LEAVE_API_NOINIT_NOLOCK(ret_value) } diff --git a/src/H5private.h b/src/H5private.h index 141fc97aca7..b7d5d9c2551 100644 --- a/src/H5private.h +++ b/src/H5private.h @@ -1453,6 +1453,26 @@ extern char H5_lib_vers_info_g[]; H5_API_LOCK \ { +/* + * Use this macro for public API functions that may be re-entered from inside + * a library callback that already holds the API lock (e.g. filter v3 + * H5Z_set_config_func_t callbacks invoked from H5Pappend_filter). Performs + * the same error-handling and function-name setup as FUNC_ENTER_API_NOINIT + * but skips H5_API_LOCK, so the function is safe to call while another + * public API frame already holds the lock. Only appropriate for pure + * helpers that touch no global library state. Examples: H5Zconfig_get_int, + * H5Zconfig_get_double, etc. + */ +#define FUNC_ENTER_API_NOINIT_NOLOCK \ + { \ + { \ + { \ + H5_CHECK_FUNCTION_NAME(H5_IS_PUBLIC(__func__)); \ + \ + H5_API_SETUP_PUBLIC_API_VARS \ + H5_API_SETUP_ERROR_HANDLING \ + { + /* * Use this macro for public API functions that shouldn't perform _any_ * initialization of the library or an interface or push themselves on the @@ -1676,6 +1696,17 @@ extern char H5_lib_vers_info_g[]; } \ } /* end scope from beginning of FUNC_ENTER */ +/* Use this macro to match the FUNC_ENTER_API_NOINIT_NOLOCK macro */ +#define FUNC_LEAVE_API_NOINIT_NOLOCK(ret_value) \ + ; \ + } /* end scope from end of FUNC_ENTER */ \ + if (H5_UNLIKELY(err_occurred)) \ + (void)H5E_dump_api_stack(); \ + return (ret_value); \ + } \ + } \ + } /* end scope from beginning of FUNC_ENTER */ + /* Use this macro to match the FUNC_ENTER_API_NOINIT_NOERR macro */ #define FUNC_LEAVE_API_NOERR(ret_value) \ ; \