Skip to content

Fix Zarr-to-HDF5 export deadlock - #363

Closed
rly wants to merge 1 commit into
zarr-v3-migrationfrom
fix/export-deadlock-materialize
Closed

Fix Zarr-to-HDF5 export deadlock#363
rly wants to merge 1 commit into
zarr-v3-migrationfrom
fix/export-deadlock-materialize

Conversation

@rly

@rly rly commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Fixes #362.

Patches zarr.Array.__array__ so numpy coercion of a zarr Array reads on the calling thread. During HDF5IO.export, h5py copies the array while holding its global phil lock; zarr's default read blocks on the shared zarr_io loop thread, and a cyclic-GC finalizer that acquires phil on that thread deadlocks against the caller. Reading on the calling thread keeps the loop thread out of the read, so the lock cycle cannot form.

Verified: an end-to-end ZarrIO→HDF5 export reproducer deadlocks 10/10 processes without this change and 0/10 with it. Adds a deterministic (non-racy) regression test.

🤖 Generated with Claude Code

Patch zarr.Array.__array__ so numpy coercion of a zarr Array reads on the
calling thread. During HDF5IO.export, h5py copies the array while holding its
global "phil" lock; zarr's default read blocks on the shared "zarr_io" loop
thread, and a cyclic-GC finalizer that acquires "phil" on that thread then
deadlocks against the caller. Reading on the calling thread keeps the loop
thread out of the read, so the lock cycle cannot form.

Fixes #362

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 84.10%. Comparing base (b3dbd30) to head (4d609ca).

Files with missing lines Patch % Lines
src/hdmf_zarr/backend.py 93.33% 1 Missing ⚠️
Additional details and impacted files
@@                  Coverage Diff                  @@
##           zarr-v3-migration     #363      +/-   ##
=====================================================
+ Coverage              84.00%   84.10%   +0.09%     
=====================================================
  Files                      5        5              
  Lines                   1388     1403      +15     
  Branches                 278      279       +1     
=====================================================
+ Hits                    1166     1180      +14     
- Misses                   146      147       +1     
  Partials                  76       76              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rly

rly commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Superseding this in favor of hdmf-dev/hdmf#1547, which fixes the deadlock in the right place.

This PR patched zarr.Array.__array__ process-wide and materialized inside the window where h5py holds phil (its global lock), which forced a fresh-per-call event loop as a workaround (untested against aiohttp-backed remote stores). hdmf#1547 instead materializes the zarr array in HDF5IO.__list_fill__/__scalar_fill__ before the write, removing the only window where phil is held across a zarr sync(). That uses zarr's normal, tested read path, is scoped to the export/write path (no process-wide behavior change), and is RAM-neutral. hdmf#1547 also makes HDMFIO.__del__ non-blocking (ResourceWarning + atexit) to remove the unsafe blocking-close-in-a-finalizer for the whole IO class.

Closing here; the fix will land via the next HDMF release (bump the hdmf>= pin then).

@rly rly closed this Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants