diff --git a/CHANGELOG.md b/CHANGELOG.md index 000f22f45..bf465a4b7 100644 --- a/CHANGELOG.md +++ b/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 `IndexError` when selecting an empty region from an in-memory `DynamicTableRegion` (e.g. `table["region"][i]` for a ragged region row that references no target rows, or an empty slice), both when the target table's columns hold their data as numpy arrays and when the target table has ragged columns. @h-mayorquin [#1549](https://github.com/hdmf-dev/hdmf/pull/1549) - 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) diff --git a/src/hdmf/backends/hdf5/h5tools.py b/src/hdmf/backends/hdf5/h5tools.py index bd2a3eaa9..de7169e21 100644 --- a/src/hdmf/backends/hdf5/h5tools.py +++ b/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: diff --git a/src/hdmf/backends/io.py b/src/hdmf/backends/io.py index 23319628f..21b936fcc 100644 --- a/src/hdmf/backends/io.py +++ b/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,27 @@ 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 not still_open: + return + + try: + source = getattr(self, "source", None) + warn( + f"{type(self).__name__} was not closed before being garbage collected. " + "Use a context manager or call close() to release resources. " + f"Source: {source!r}", + ResourceWarning, + ) + except Exception: + pass diff --git a/tests/unit/test_io_hdf5_h5tools.py b/tests/unit/test_io_hdf5_h5tools.py index c75a6eb82..8fbc58bef 100644 --- a/tests/unit/test_io_hdf5_h5tools.py +++ b/tests/unit/test_io_hdf5_h5tools.py @@ -1,7 +1,9 @@ """Test module to validate that HDF5IO is working""" +import gc import os import unittest import warnings +import weakref from io import BytesIO from pathlib import Path import shutil @@ -4516,3 +4518,56 @@ 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): + import hdmf.backends.io as io_module + + # Isolate the registry so the simulated cleanup only touches the IO created here, + # not IOs left open by other tests (which would couple test order and mask leaks). + original_open_ios = io_module._open_ios + io_module._open_ios = weakref.WeakSet() + try: + io = HDF5IO(self.path, manager=self.manager, mode='w') + self.assertIn(io, io_module._open_ios) + self.assertTrue(io.is_open()) + io_module._close_open_ios() # simulate interpreter-exit cleanup + self.assertFalse(io.is_open()) + finally: + io_module._open_ios = original_open_ios