From 17edfe2cf0c2e56311f43337016e4bc72902df14 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 09:03:53 -0500 Subject: [PATCH] test(py): detect a held GIL, and errors h5py does not raise test_threads_read_the_same_file passed with the GIL held. The new test_reads_release_the_gil measures the longest stall of a spinning Python thread while another reads: with py.detach removed from the read it stalled 0.062 s of a 0.064 s read and failed; with it, about 3 ms. test_errors_match_h5py now compares the result whenever h5py reads the key, instead of only checking that we raise when h5py raises, over a longer key list. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 8 +++ crates/clawhdf5-py/tests/test_read_vs_h5py.py | 62 +++++++++++++++++-- 2 files changed, 65 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 78a3628..e175c02 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -94,6 +94,14 @@ - **`ds[np.array(1)]` is an integer index**, as in h5py; a 0-d integer array went down the index-list path and raised a confusing `TypeError`. The h5py comparison keys now include 0-d arrays on every axis. +- **Tests that would notice a held GIL, and our extra errors.** + `test_reads_release_the_gil` times a Python thread spinning while another + reads: with the read made to hold the GIL it stalls for the whole read + (0.062 s of a 0.064 s read) and the test fails; released, its longest + stall is about 3 ms. (The existing threads test only checked values.) + `test_errors_match_h5py` now also requires that every key h5py reads + reads here too, with the same result, and covers more keys (0-d arrays, + repeated and empty lists, `()`, `...`). - **CI builds and tests the Python package.** It was excluded from CI. `scripts/ci-test.sh` now lints `clawhdf5-py`, builds the wheel with maturin, unpacks it under `target/` and runs the pytest suite; skipped diff --git a/crates/clawhdf5-py/tests/test_read_vs_h5py.py b/crates/clawhdf5-py/tests/test_read_vs_h5py.py index cbac1e4..6a3b6cb 100644 --- a/crates/clawhdf5-py/tests/test_read_vs_h5py.py +++ b/crates/clawhdf5-py/tests/test_read_vs_h5py.py @@ -266,7 +266,10 @@ def keys_for(shape): # checked against the values written instead (test_vlen_big_endian). H5PY_MISREADS = {"vlen/f8_be"} -ERROR_KEYS_1D = [slice(None, None, -1), 10**6, -(10**6), None, (0, 0, 0, 0, 0), [3, 1], (Ellipsis, Ellipsis), 1.5, "nope"] +ERROR_KEYS_1D = [ + slice(None, None, -1), 10**6, -(10**6), None, (0, 0, 0, 0, 0), [3, 1], (Ellipsis, Ellipsis), 1.5, "nope", + np.array(1.0), np.array(True), np.array(10**6), [0, 0], [], (), Ellipsis, (0,), [-1], np.array([1, 2]), +] # --------------------------------------------------------------------------- @@ -322,12 +325,13 @@ def test_errors_match_h5py(h5py, pair): for name in ["num/le_i4_1d", "num/be_f8_2d_gzip", "str/vlen", "num/scalar_f8"]: for key in ERROR_KEYS_1D: try: - theirs[name][key] + expected = theirs[name][key] except Exception as e: # noqa: BLE001 with pytest.raises(type(e)): ours[name][key] else: - pass # valid for this shape; covered by the value test + # h5py reads it, so must we (and the same values). + assert_same(ours[name][key], expected, f"{name}[{key!r}]") def test_compound_fields_match_h5py(pair): @@ -454,8 +458,7 @@ def test_only_the_selected_chunks_are_read(h5py, tmp_path): def test_threads_read_the_same_file(pair): - """Reads from many threads at once (the GIL is released during each - read) return exactly what h5py returns.""" + """Reads from many threads at once return exactly what h5py returns.""" ours, theirs, _ = pair names = ["num/le_f8_2d_gzip", "num/i4_3d_shuffle", "str/vlen_2d_gzip", "cmp/padded_chunked", "num/be_i8_1d"] expected = {n: theirs[n][()] for n in names} @@ -477,6 +480,55 @@ def test_threads_read_the_same_file(pair): assert not errors, errors[:3] +def test_reads_release_the_gil(h5py, tmp_path): + """While one thread is inside a long read, another Python thread keeps + running. With the GIL held for the read, the other thread would stall + for the whole read; the test measures its longest stall.""" + import sys + import time + + path = str(tmp_path / "gil.h5") + data = np.arange(2048 * 4096, dtype=" 0.03, f"a read took only {one_read:.3f} s; too short to measure" + + old = sys.getswitchinterval() + sys.setswitchinterval(0.001) + stop = threading.Event() + gaps = [] + + def spin(): + last = time.perf_counter() + worst = 0.0 + while not stop.is_set(): + now = time.perf_counter() + worst = max(worst, now - last) + last = now + gaps.append(worst) + + try: + t = threading.Thread(target=spin) + t.start() + time.sleep(0.01) + t0 = time.perf_counter() + for _ in range(2): + ds[key] + reading = time.perf_counter() - t0 + stop.set() + t.join() + finally: + sys.setswitchinterval(old) + np.testing.assert_array_equal(ds[key], data[:1000]) + # Held, the spinner would stall for about one read. + assert gaps[0] < one_read / 3, f"spinner stalled {gaps[0]:.3f} s during reads of {one_read:.3f} s ({reading:.3f} s)" + + def _v4_index_fixture(h5py, path): """One 2-D dataset per v4 chunk index (HDF5 1.10+ layout, libver='latest').""" data = (np.arange(37 * 23, dtype="