Avoid Zarr-to-HDF5 export deadlock and make the IO finalizer non-blocking - #1547
Open
rly wants to merge 10 commits into
Open
Avoid Zarr-to-HDF5 export deadlock and make the IO finalizer non-blocking#1547rly wants to merge 10 commits into
rly wants to merge 10 commits into
Conversation
…king Read a zarr array into memory in HDF5IO.__list_fill__/__scalar_fill__ before the HDF5 write, so the read does not run while h5py's global lock is held. zarr v3 dispatches reads to a background event-loop thread; reading under that lock can deadlock against a garbage-collection finalizer that acquires the same lock on that thread. Make HDMFIO.__del__ non-blocking: a finalizer that runs a lock-acquiring close on an unpredictable thread can deadlock or hit interpreter-shutdown ordering issues. __del__ now emits a ResourceWarning for an unclosed IO (as CPython does for files), and still-open IOs are flushed and closed by an atexit handler on the main thread. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1547 +/- ##
==========================================
- Coverage 93.22% 93.15% -0.08%
==========================================
Files 41 41
Lines 10221 10245 +24
Branches 2115 2119 +4
==========================================
+ Hits 9529 9544 +15
- Misses 415 422 +7
- Partials 277 279 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The test's skipUnless decorator referenced zarr.__version__ at class-body evaluation time, raising NameError during collection in environments where zarr is not installed. The test was also correctness-neutral (the materialize change prevents a concurrency deadlock, not a wrong result) and always skipped under the zarr v2 pin used in CI, so it provided no regression coverage. The materialize code path stays covered by the existing zarr-input round-trip tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a Zarr→HDF5 export deadlock by ensuring Zarr arrays are materialized before HDF5 writes, and makes HDMFIO finalization non-blocking by warning on unclosed IOs and deferring best-effort cleanup to an atexit handler.
Changes:
- Materialize Zarr arrays (
np.asarray) beforecreate_dataset/dset[:] =to avoid reads occurring while the HDF5 global lock is held. - Replace
HDMFIO.__del__blockingclose()with aResourceWarningand add anatexitcleanup hook that closes still-tracked IO instances. - Add regression tests covering the new finalizer behavior and
atexitcleanup registration.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/unit/test_io_hdf5_h5tools.py | Adds tests for HDMFIO.__del__ warning behavior and atexit cleanup registration. |
| src/hdmf/backends/io.py | Introduces WeakSet tracking of open IOs, atexit cleanup, and changes __del__ to warn instead of closing. |
| src/hdmf/backends/hdf5/h5tools.py | Materializes Zarr arrays before HDF5 dataset creation/assignment to avoid deadlocks. |
| CHANGELOG.md | Documents the finalizer/atexit behavior change and the Zarr→HDF5 deadlock fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Swap a fresh WeakSet into hdmf.backends.io._open_ios for the duration of the test so the simulated interpreter-exit cleanup only closes the IO created here, not IOs left open by other tests (addresses a PR review comment). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two related changes to remove a deadlock when exporting a Zarr v3 file to HDF5. Context: hdmf-dev/hdmf-zarr#362.
Materialize before the HDF5 write.
HDF5IO.__list_fill__/__scalar_fill__read a zarr array into memory beforecreate_dataset/dset[:] =, so the read does not run while h5py's global lock is held. zarr v3 dispatches reads to a background event-loop thread; reading under that lock can deadlock against a GC finalizer that acquires the same lock on that thread. Scoped to zarr arrays via the existingis_zarr_arrayhelper; RAM-neutral (h5py already materializes on assignment).Non-blocking IO finalizer.
HDMFIO.__del__no longer runs a lock-acquiringclose()(finalizers run at unpredictable times on unpredictable threads, so a blocking close can deadlock or hit shutdown-ordering issues). It now emits aResourceWarningfor an unclosed IO, as CPython does for files; still-open IOs are flushed and closed by anatexithandler on the main thread.The materialize change alone resolves the deadlock; the finalizer change removes the unsafe blocking-close-in-a-finalizer pattern for the whole IO class.
Verified against a concurrent Zarr→HDF5 export reproducer (10/10 processes deadlock without the materialize change, 0/10 with it). Adds regression tests for both changes. No new failures in the existing suite (pre-existing zarr-v3 API failures in
TestWriteHDF5withZarrInputetc. are unrelated).🤖 Generated with Claude Code