Background
Follow-up from review discussion on #71 (#71 (comment)).
`H5_DAOS_G_INIT()` (src/daos_vol_private.h) calls `H5VLget_connector_id_by_value()` to ensure `H5_DAOS_g` is set. Per the HDF5 docs, this is a "get" call that returns a new reference and should be released with `H5VLclose()` once no longer needed - unlike the older "peek" form, which returned the ID without taking a reference.
`H5_daos_term()` (src/daos_vol.c) intentionally does not release this reference. It was tried (244ea63) and reverted (2596e9d) because `H5_daos_term()` is itself invoked while closing the last reference to that same ID, so decrementing it again from inside the callback recurses into still-being-torn-down library state - this was reproduced as a 100%-reproducible DAOS task scheduler assertion abort (`tse_sched_progress: Assertion '0' failed`) on process shutdown in CI.
The gap
Per @jhendersonHDF:
Though it's very much an edge case, this is still a leak and needs to be closed in some cases to be clean. The termination function could be called outside of just the library terminating, which would leak the ID and also leave an incremented reference count on the associated connector object structure. [...] `H5is_library_terminating()` might be usable here, though that does seem like hacking around design issues and it also still has to be known whether `H5_DAOS_g` was actually initialized by `H5VLget_connector_id_by_value()` or somewhere else.
So a correct fix needs two things neither of which exist today:
- A way to distinguish "H5_daos_term() fired because the library is doing a forced full shutdown" (safe to leave the reference - HDF5 tears the atom down regardless of refcount) from "H5_daos_term() fired via a normal refcounted close reaching zero outside of shutdown" (the reference should actually be released, and it's safe to do so since we're not racing our own atom's forced teardown) - candidate: `H5is_library_terminating()`, with the caveat that it may itself be hacking around a design issue in the VOL terminate-callback contract.
- Provenance tracking for `H5_DAOS_g`: it can currently be set via `H5_DAOS_G_INIT()`'s `H5VLget_connector_id_by_value()` call, or via `H5Pset_fapl_daos()`'s `H5VLregister_connector()` / `H5VLget_connector_id_by_name()` paths, which have different ownership semantics. Only the `H5_DAOS_G_INIT()` case is the one this issue is about.
Why not fixed now
Confirmed low priority by @jhendersonHDF ("Probably not worth worrying about right now, but it is a design problem that could cause issues"), and a wrong attempt here has already caused a real, 100%-reproducible crash in CI. Filing this so the reasoning and the two missing pieces aren't lost.
Background
Follow-up from review discussion on #71 (#71 (comment)).
`H5_DAOS_G_INIT()` (src/daos_vol_private.h) calls `H5VLget_connector_id_by_value()` to ensure `H5_DAOS_g` is set. Per the HDF5 docs, this is a "get" call that returns a new reference and should be released with `H5VLclose()` once no longer needed - unlike the older "peek" form, which returned the ID without taking a reference.
`H5_daos_term()` (src/daos_vol.c) intentionally does not release this reference. It was tried (244ea63) and reverted (2596e9d) because `H5_daos_term()` is itself invoked while closing the last reference to that same ID, so decrementing it again from inside the callback recurses into still-being-torn-down library state - this was reproduced as a 100%-reproducible DAOS task scheduler assertion abort (`tse_sched_progress: Assertion '0' failed`) on process shutdown in CI.
The gap
Per @jhendersonHDF:
So a correct fix needs two things neither of which exist today:
Why not fixed now
Confirmed low priority by @jhendersonHDF ("Probably not worth worrying about right now, but it is a design problem that could cause issues"), and a wrong attempt here has already caused a real, 100%-reproducible crash in CI. Filing this so the reasoning and the two missing pieces aren't lost.