From 7154260222243935a5524313206abad762269556 Mon Sep 17 00:00:00 2001 From: Othman El Hammouchi Date: Fri, 21 Nov 2025 00:51:44 +0100 Subject: [PATCH 1/4] fix: ensure `ZipStore` is open before acquiring lock --- src/zarr/storage/_zip.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/zarr/storage/_zip.py b/src/zarr/storage/_zip.py index 72bf9e335a..bb32fb7ead 100644 --- a/src/zarr/storage/_zip.py +++ b/src/zarr/storage/_zip.py @@ -120,12 +120,18 @@ def __setstate__(self, state: dict[str, Any]) -> None: def close(self) -> None: # docstring inherited + if not self._is_open: + self._sync_open() + super().close() with self._lock: self._zf.close() async def clear(self) -> None: # docstring inherited + if not self._is_open: + self._sync_open() + with self._lock: self._check_writable() self._zf.close() @@ -188,6 +194,8 @@ async def get_partial_values( key_ranges: Iterable[tuple[str, ByteRequest | None]], ) -> list[Buffer | None]: # docstring inherited + if not self._is_open: + self._sync_open() out = [] with self._lock: for key, byte_range in key_ranges: @@ -222,6 +230,9 @@ async def set(self, key: str, value: Buffer) -> None: async def set_if_not_exists(self, key: str, value: Buffer) -> None: self._check_writable() + if not self._is_open: + self._sync_open() + with self._lock: members = self._zf.namelist() if key not in members: @@ -245,6 +256,9 @@ async def delete(self, key: str) -> None: async def exists(self, key: str) -> bool: # docstring inherited + if not self._is_open: + self._sync_open() + with self._lock: try: self._zf.getinfo(key) @@ -255,6 +269,9 @@ async def exists(self, key: str) -> bool: async def list(self) -> AsyncIterator[str]: # docstring inherited + if not self._is_open: + self._sync_open() + with self._lock: for key in self._zf.namelist(): yield key From 35f801963042cf4cfd5fc56003a5b640144d4f08 Mon Sep 17 00:00:00 2001 From: Othman El Hammouchi Date: Thu, 25 Dec 2025 12:18:08 +0100 Subject: [PATCH 2/4] refactor: Cleanup open check in ZipStore --- src/zarr/storage/_zip.py | 33 ++++++++++++++------------------- 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/src/zarr/storage/_zip.py b/src/zarr/storage/_zip.py index bb32fb7ead..776f07e39f 100644 --- a/src/zarr/storage/_zip.py +++ b/src/zarr/storage/_zip.py @@ -103,6 +103,10 @@ def _sync_open(self) -> None: self._is_open = True + def _sync_ensure_open(self): + if not self._is_open: + self._sync_open() + async def _open(self) -> None: self._sync_open() @@ -120,17 +124,15 @@ def __setstate__(self, state: dict[str, Any]) -> None: def close(self) -> None: # docstring inherited - if not self._is_open: - self._sync_open() - + self._sync_ensure_open() + super().close() with self._lock: self._zf.close() async def clear(self) -> None: # docstring inherited - if not self._is_open: - self._sync_open() + self._sync_ensure_open() with self._lock: self._check_writable() @@ -155,8 +157,7 @@ def _get( prototype: BufferPrototype, byte_range: ByteRequest | None = None, ) -> Buffer | None: - if not self._is_open: - self._sync_open() + self._sync_ensure_open() # docstring inherited try: with self._zf.open(key) as f: # will raise KeyError @@ -194,8 +195,7 @@ async def get_partial_values( key_ranges: Iterable[tuple[str, ByteRequest | None]], ) -> list[Buffer | None]: # docstring inherited - if not self._is_open: - self._sync_open() + self._sync_ensure_open() out = [] with self._lock: for key, byte_range in key_ranges: @@ -203,8 +203,7 @@ async def get_partial_values( return out def _set(self, key: str, value: Buffer) -> None: - if not self._is_open: - self._sync_open() + self._sync_ensure_open() # generally, this should be called inside a lock keyinfo = zipfile.ZipInfo(filename=key, date_time=time.localtime(time.time())[:6]) keyinfo.compress_type = self.compression @@ -218,8 +217,7 @@ def _set(self, key: str, value: Buffer) -> None: async def set(self, key: str, value: Buffer) -> None: # docstring inherited self._check_writable() - if not self._is_open: - self._sync_open() + self._sync_ensure_open() assert isinstance(key, str) if not isinstance(value, Buffer): raise TypeError( @@ -230,8 +228,7 @@ async def set(self, key: str, value: Buffer) -> None: async def set_if_not_exists(self, key: str, value: Buffer) -> None: self._check_writable() - if not self._is_open: - self._sync_open() + self._sync_ensure_open() with self._lock: members = self._zf.namelist() @@ -256,8 +253,7 @@ async def delete(self, key: str) -> None: async def exists(self, key: str) -> bool: # docstring inherited - if not self._is_open: - self._sync_open() + self._sync_ensure_open() with self._lock: try: @@ -269,8 +265,7 @@ async def exists(self, key: str) -> bool: async def list(self) -> AsyncIterator[str]: # docstring inherited - if not self._is_open: - self._sync_open() + self._sync_ensure_open() with self._lock: for key in self._zf.namelist(): From 05970a2a528e332edfdedfc7601becfe83039e9a Mon Sep 17 00:00:00 2001 From: Othman El Hammouchi Date: Thu, 25 Dec 2025 12:43:06 +0100 Subject: [PATCH 3/4] test: Add test to check that lock is present when required --- tests/test_store/test_zip.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/test_store/test_zip.py b/tests/test_store/test_zip.py index 744ee82945..7bfc4cfb0c 100644 --- a/tests/test_store/test_zip.py +++ b/tests/test_store/test_zip.py @@ -152,3 +152,17 @@ async def test_move(self, tmp_path: Path) -> None: assert destination.exists() assert not origin.exists() assert np.array_equal(array[...], np.arange(10)) + + async def test_lock_present(self, store: ZipStore) -> None: + buf = cpu.Buffer.from_bytes(b"bar") + await store.set("foo", buf) + await store.set_if_not_exists("foo", buf) + await store.exists("foo") + await store.get("foo", default_buffer_prototype()) + + async for _ in store.list(): + pass + + await store.clear() + + store.close() From 864e445aa733593b25fb559f34365ebcc1725e3e Mon Sep 17 00:00:00 2001 From: Othman El Hammouchi Date: Thu, 25 Dec 2025 12:56:17 +0100 Subject: [PATCH 4/4] refactor: Add return type to `_sync_ensure_open` --- src/zarr/storage/_zip.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/zarr/storage/_zip.py b/src/zarr/storage/_zip.py index 776f07e39f..bc6677043f 100644 --- a/src/zarr/storage/_zip.py +++ b/src/zarr/storage/_zip.py @@ -103,7 +103,7 @@ def _sync_open(self) -> None: self._is_open = True - def _sync_ensure_open(self): + def _sync_ensure_open(self) -> None: if not self._is_open: self._sync_open()