Avoid Zarr-to-HDF5 export deadlock and make the IO finalizer non-blocking

CHANGELOG.md

@@ -9,8 +9,10 @@
 - Added `hdmf.build.ObjectMapper.NO_OVERRIDE`, a sentinel that a `constructor_arg` or `object_attr` override function returns to fall through to the value built from the file or read from the container. Returning `None` from an override function to signal "no override" is deprecated: it still falls through but now emits a `DeprecationWarning`. In HDMF 8.0, a `None` return will set the constructor argument or attribute to `None`. @rly [#1167](https://github.com/hdmf-dev/hdmf/pull/1167)
 - Added support for hdmf-common schema 1.10.0, which changes ``MeaningsTable.target`` from a link to an object-reference attribute (``dtype`` with ``reftype: object``, ``target_type: VectorData``). Files written with the hdmf-common 1.9.0 ``MeaningsTable`` (a link named "target") are still read correctly via a backwards-compatibility mapping in ``MeaningsTableMap``. There is no change in the read/write API; the change is limited to the representation on disk. @rly [#1525](https://github.com/hdmf-dev/hdmf/pull/1525)
 - ``DynamicTable.get_meanings_for_column`` (and the ``AlignedDynamicTable`` override) now returns ``None`` when the named column exists but has no ``MeaningsTable``, and raises ``KeyError`` only when the column itself does not exist. @rly [#1538](https://github.com/hdmf-dev/hdmf/pull/1538)
+- `HDMFIO.__del__` emits a `ResourceWarning` for an IO that was not closed instead of calling `close()`. Still-open IOs are flushed and closed by an `atexit` handler on the main thread. @rly [#1547](https://github.com/hdmf-dev/hdmf/pull/1547)
 
 ### Fixed
+- Fixed a deadlock when exporting a Zarr file to HDF5 with `HDF5IO.export`. A zarr array is now read into memory before the HDF5 write (in `__list_fill__`/`__scalar_fill__`), so the read does not run while h5py's global lock is held. @rly [#1547](https://github.com/hdmf-dev/hdmf/pull/1547)
 - Fixed the Jupyter HTML representation (`_repr_html_`) rendering a scalar numpy `bool` or `int` attribute (e.g. `np.bool_`, `np.int64`, as read back from an HDF5 attribute) as an expandable "array" block reporting `Shape: ()`, while `float` and `str` scalars rendered inline. `_unwrap_scalar` now also unwraps numpy scalars (`np.generic`) so every scalar renders inline consistently. @h-mayorquin [#1546](https://github.com/hdmf-dev/hdmf/pull/1546)
 - Fixed subclassing a `MultiContainerInterface` type without redefining `__init__`. A subclass now inherits the ancestor's constructor (custom or auto-generated) instead of regenerating one that drops the parent's docval arguments. This restores the common "generate with `get_class`, subclass to customize one method, re-register with `@register_class`" extension idiom for `DynamicTable`-family subtypes. @rly [#1540](https://github.com/hdmf-dev/hdmf/pull/1540)
 - Fixed `ObjectMapper._parse_isoformat` raising `ValueError: Invalid isoformat string` when reading an ISO 8601 timestamp that ends in the `Z` (UTC) designator on Python < 3.11. `datetime.fromisoformat` did not accept a trailing `Z` until Python 3.11, but `requires-python` is `>=3.10` and hdmf's own writer emits the equivalent `+00:00` offset, so a `Z`-terminated timestamp written by a peer tool was unreadable on the supported 3.10 floor. A trailing `Z`/`z` is now normalized to `+00:00` before parsing. @Leonard013 [#1539](https://github.com/hdmf-dev/hdmf/pull/1539)

src/hdmf/backends/hdf5/h5tools.py

@@ -1299,6 +1299,10 @@ def __scalar_fill__(cls, parent, name, data, options=None):
             except Exception as exc:
                 msg = 'cannot add %s to %s - could not determine type' % (name, parent.name)
                 raise Exception(msg) from exc
+        # Read a zarr array into memory before creating the dataset so its read does not
+        # run while the HDF5 lock is held (see __list_fill__ for the deadlock this avoids).
+        if is_zarr_array(data):
+            data = np.asarray(data)
         try:
             dset = parent.create_dataset(name, data=data, shape=None, dtype=dtype, **io_settings)
         except Exception as exc:
@@ -1447,6 +1451,12 @@ def __list_fill__(cls, parent, name, data, options=None):
             new_shape = list(dset.shape)
             new_shape[0] = _get_length(data)
             dset.resize(new_shape)
+        # Read a zarr array into memory before the assignment so its read does not run
+        # while the HDF5 lock is held. zarr v3 dispatches reads to a background
+        # event-loop thread; reading under the HDF5 lock can deadlock against a garbage
+        # collection finalizer that acquires the same lock on that thread.
+        if is_zarr_array(data):
+            data = np.asarray(data)
         try:
             dset[:] = data
         except Exception as e:

src/hdmf/backends/io.py

@@ -1,5 +1,7 @@
 from abc import ABCMeta, abstractmethod
+import atexit
 import os
+import weakref
 from pathlib import Path
 
 from ..build import BuildManager, GroupBuilder, TypeMap
@@ -10,6 +12,21 @@
 from warnings import warn
 
 
+# Track open HDMFIO instances so their files can be flushed and closed at interpreter
+# exit if the user forgot to close them. Cleanup runs here on the main thread at a
+# controlled time, where a blocking close cannot deadlock.
+_open_ios = weakref.WeakSet()
+
+
+@atexit.register
+def _close_open_ios():
+    for io in list(_open_ios):
+        try:
+            io.close()
+        except Exception:  # pragma: no cover - best-effort cleanup at interpreter exit
+            pass
+
+
 class HDMFIO(metaclass=ABCMeta):
 
     @docval({'name': 'manager', 'type': BuildManager,
@@ -33,6 +50,7 @@ def __init__(self, **kwargs):
         self.herd_path = herd_path
         self.herd = None
         self.open()
+        _open_ios.add(self)
 
     @property
     def manager(self):
@@ -241,4 +259,21 @@ def __exit__(self, type, value, traceback):
         self.close()
 
     def __del__(self):
-        self.close()
+        # Do not close here. close() can acquire a lock (e.g. h5py's global lock), and
+        # finalizers run at unpredictable times on unpredictable threads (a background
+        # event-loop thread, or the main thread mid garbage collection), where a
+        # blocking, lock-acquiring close can deadlock. Surface the unclosed resource the
+        # way CPython does for files; _close_open_ios() and the backend's own file
+        # finalizer release the handle.
+        is_open = getattr(self, "is_open", None)
+        try:
+            still_open = bool(is_open()) if callable(is_open) else False
+        except Exception:
+            still_open = False
+        if still_open:
+            warn(
+                f"{type(self).__name__} was not closed before being garbage collected. "
+                "Use a context manager or call close() to release resources. "
+                f"Source: {self.source!r}",
+                ResourceWarning,
+            )

tests/unit/test_io_hdf5_h5tools.py

@@ -1,4 +1,5 @@
 """Test module to validate that HDF5IO is working"""
+import gc
 import os
 import unittest
 import warnings
@@ -4516,3 +4517,49 @@ def test_subclasses_are_expandable(self):
             # VectorData subclass (custom_col) should be expandable
             self.assertEqual(f['custom_col'].maxshape, (None,))
             self.assertIsNotNone(f['custom_col'].chunks)
+
+
+class TestHDMFIOFinalizer(TestCase):
+    """The IO finalizer surfaces an unclosed IO without a blocking close.
+
+    close() can acquire a lock that a garbage-collection finalizer must not block on, so
+    __del__ warns instead of closing. Cleanup of a still-open IO is deferred to an atexit
+    handler that runs on the main thread.
+    """
+
+    def setUp(self):
+        self.manager = get_foo_buildmanager()
+        self.path = get_temp_filepath()
+
+    def tearDown(self):
+        if os.path.exists(self.path):
+            os.remove(self.path)
+
+    def test_del_warns_when_still_open(self):
+        io = HDF5IO(self.path, manager=self.manager, mode='w')
+        with warnings.catch_warnings(record=True) as recorded:
+            warnings.simplefilter("always")
+            del io
+            gc.collect()
+        resource_warnings = [w for w in recorded if issubclass(w.category, ResourceWarning)]
+        self.assertEqual(len(resource_warnings), 1)
+        self.assertIn("was not closed", str(resource_warnings[0].message))
+
+    def test_del_does_not_warn_after_close(self):
+        io = HDF5IO(self.path, manager=self.manager, mode='w')
+        io.close()
+        with warnings.catch_warnings(record=True) as recorded:
+            warnings.simplefilter("always")
+            del io
+            gc.collect()
+        resource_warnings = [w for w in recorded if issubclass(w.category, ResourceWarning)]
+        self.assertEqual(len(resource_warnings), 0)
+
+    def test_open_io_registered_for_atexit_cleanup(self):
+        from hdmf.backends.io import _open_ios, _close_open_ios
+
+        io = HDF5IO(self.path, manager=self.manager, mode='w')
+        self.assertIn(io, _open_ios)
+        self.assertTrue(io.is_open())
+        _close_open_ios()  # simulate interpreter-exit cleanup
+        self.assertFalse(io.is_open())