Close datatype IDs derived from memory type in the JNI translation helpers - #6594
Close datatype IDs derived from memory type in the JNI translation helpers#6594mattjala wants to merge 6 commits into
Conversation
…helpers The object-tree read/write helpers in h5util.c derive a base datatype from the memory type with H5Tget_super() for the variable-length, array and complex classes, but never closed it. Because an hid_t is not reclaimed when the native method returns, every read or write of such data leaked at least one datatype ID for the lifetime of the process, and nested types leaked one per level. This PR updates the helpers to close the derived type in their done: blocks, which covers both the success and the error paths, and to reset the id in the compound loops to avoid the potential for double closes. It also has the helpers release the class references that the per-element helpers look up on entry. These are local references, so they were reclaimed when the enclosing native method returned, but a compound read calls the helper once per member per element and held one set per call until then. Releasing them at the single exit bounds the count of references to the recursion depth.
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes JNI helper leaks by ensuring derived HDF5 datatype IDs and JNI local references are released on all paths, and adds regression tests to prevent reintroduction.
Changes:
- Close transient datatype IDs derived via
H5Tget_super()/ member-type lookups indone:blocks and prevent potential double-closes. - Release per-call JNI local class references in translation helpers to avoid excessive local ref accumulation in deep/compound recursion.
- Add JUnit regression tests that assert open datatype ID counts remain stable across repeated read/write cycles.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| java/src-jni/jni/h5util.c | Closes derived/member datatype IDs on exit paths and deletes JNI local refs to prevent leaks/overflow during recursion. |
| java/src-jni/test/TestH5D.java | Adds tests that loop read/write operations and assert datatype ID counts do not grow (VL, array, compound-of-VL). |
| java/src-jni/test/testfiles/JUnit-TestH5D.txt | Updates expected JUnit output to include the newly added tests and updated totals. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Review ChecklistThis PR touches the following areas. Each needs a sign-off
Additional reviewers (not owners of a touched area): @bmribler |
| done: | ||
| /* The base type derived from a VLEN/ARRAY/COMPLEX memory type, or the member type of | ||
| * a COMPOUND that has not been closed by the loop above, is owned by this call. */ | ||
| if (memb >= 0) |
There was a problem hiding this comment.
H5Tclose(0) will fail, right?
There was a problem hiding this comment.
True. memb shouldn't be able to be zero exactly here, but we may as well be precise. I'll update this check and some similar ones.
There was a problem hiding this comment.
I don't believe it's guaranteed anywhere that the ID won't be 0 or that H5Tclose(0) will fail. The H5I code called when closing also asserts id >= 0 and checking for >= 0 is a fairly common pattern throughout the library.
There was a problem hiding this comment.
H5Tclose(0) does fail at present, but that's a consequence of how the hash table is set up presently and, I suppose, not something we want to design against. Reverted to >= 0 for consistency.
There was a problem hiding this comment.
A direct test of H5Tclose(0) would fail if there is no open datatype object that was assigned an ID of 0. Whether or not an ID can actually ever be 0 is something I don't currently know, but at least the library code seems designed to accept that this could happen.
github-project-automation
Bot
moved this from To be triaged
to In progress
in HDF5 - TRIAGE & TRACK
hid_t 0 is not a valid datatype ID, so H5Tclose(0) would fail.
…ve IDs" An hid_t of 0 not being a valid ID is a property of the current H5I encoding rather than a documented guarantee, so the JNI helpers should not depend on it.
The object-tree read/write helpers in h5util.c derive a base datatype from the memory type with H5Tget_super() for the variable-length, array and complex classes, but never closed it. Because an hid_t is not reclaimed when the native method returns, every read or write of such data leaked at least one datatype ID for the lifetime of the process, and nested types leaked one per level.
This PR updates the helpers to close the derived type in their done: blocks, which covers both the success and the error paths, and to reset the id in the compound loops to avoid the potential for double closes.
It also has the helpers release the class references that the per-element helpers look up on entry. These are local references, so they were reclaimed when the enclosing native method returned, but a compound read calls the helper once per member per element and held one set per call until then. Releasing them at the single exit bounds the count of references to the recursion depth.
Fixes #6592