Skip to content

Commit 01dfef5

Browse files
FrancescAltedclaude
andcommitted
Fix aget_chunk against real async filesystems
Pointed at a live S3 endpoint for the first time (moto server + s3fs), the async path failed on every chunk: HTTPClientError: ... got Future <...> attached to a different loop fsspec drives an async filesystem's coroutines on a private event loop of its own, in a background thread. Awaiting fs._cat_file() from the caller's loop therefore uses a client built on one loop from another, which aiobotocore rejects. Its blocking API is the supported way in, so aget_chunk now hands get_chunk to a worker thread: that call dispatches to fsspec's own loop, so the thread parks on a queue rather than on a socket, and afetch() keeps overlapping fetches as before. memory:// cannot catch this -- it is not an async backend, so aget_chunk took the fallback branch there and everything passed. The test now asserts the mechanism instead of only the result: afetch must reach get_chunk, which fails if anyone awaits the filesystem coroutine again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 090e149 commit 01dfef5

3 files changed

Lines changed: 31 additions & 14 deletions

File tree

bench/ndarray/fsspec-concurrency.py

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,9 @@
1515
hide) but the gain never was, because nothing that runs offline has a round trip
1616
to hide. This script answers it against a real endpoint.
1717
18-
It also runs ``afetch()``, whose async path (``aget_chunk`` -> ``fs._cat_file``)
19-
has never executed at all: ``memory://`` is not an async backend, so only its
20-
blocking fallback is covered by the test suite.
18+
It also runs ``afetch()``, which is worth keeping in the sweep: the first time
19+
this script was pointed at a real S3 endpoint, that path failed outright, and
20+
``memory://`` cannot reproduce it because it is not an async backend.
2121
2222
Usage
2323
-----
@@ -159,8 +159,7 @@ def main():
159159
)
160160

161161
if not args.skip_afetch:
162-
# The async path, which no test has ever run: aget_chunk reaches
163-
# fs._cat_file directly rather than falling back to the blocking read
162+
# The async path, which only a real async backend exercises
164163
report(
165164
"slice, afetch (async path)",
166165
{c: timed(args.urlpath, item, c, use_afetch=True) for c in levels},

src/blosc2/proxy.py

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -793,21 +793,25 @@ def get_chunk(self, nchunk: int) -> bytes:
793793
return data[: struct.unpack("<i", data[12:16])[0]]
794794

795795
async def aget_chunk(self, nchunk: int) -> bytes:
796-
"""Same as :meth:`get_chunk`, but letting several fetches overlap.
796+
"""Same as :meth:`get_chunk`, but without blocking the caller's event loop.
797797
798798
This is what makes :meth:`Proxy.afetch` worth using against an object
799799
store, where a slice spanning many chunks is nearly all round-trip
800-
latency. Backends without an async implementation fall back to the
801-
blocking path, which costs nothing but gains nothing either.
800+
latency.
801+
802+
The fetch goes to a worker thread rather than being awaited directly.
803+
Awaiting an async filesystem's coroutine looks like the obvious thing to
804+
do and does not work: fsspec drives those on a private event loop of its
805+
own, so a client created there and awaited here raises "got Future
806+
attached to a different loop" (seen with s3fs). Its blocking API is the
807+
supported way in, and it hands off to that same private loop, so the
808+
thread parks on a queue rather than on a socket.
802809
"""
803810
offset = int(self._offsets[nchunk])
804811
if offset < 0:
805812
return self._special_chunk(offset)
806-
if not getattr(self._fs, "async_impl", False):
807-
return self.get_chunk(nchunk)
808-
end = offset + int(self._extents[nchunk])
809-
data = await self._fs._cat_file(self._path, start=offset, end=end)
810-
return data[: struct.unpack("<i", data[12:16])[0]]
813+
loop = asyncio.get_running_loop()
814+
return await loop.run_in_executor(None, self.get_chunk, nchunk)
811815

812816
def _special_chunk(self, offset: int) -> bytes:
813817
"""Rebuild a run-length chunk, which lives in its offset instead of the file."""

tests/test_fsspec.py

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -310,13 +310,27 @@ def test_lazy_fetches_only_touched_chunks(monkeypatch):
310310
assert fetched == [1, 2]
311311

312312

313-
def test_lazy_afetch():
313+
def test_lazy_afetch(monkeypatch):
314314
import asyncio
315315

316316
a = blosc2.arange(0, 1000, dtype="i4", chunks=(100,))
317317
p = blosc2.open(_put("afetch.b2nd", a), lazy=True)
318+
319+
# aget_chunk must go through the blocking get_chunk in a worker thread.
320+
# Awaiting an async filesystem's own coroutine instead raises "got Future
321+
# attached to a different loop" on s3fs, which memory:// cannot reproduce
322+
# because it is not an async backend at all
323+
fetched = []
324+
orig = blosc2.FsspecNDSource.get_chunk
325+
monkeypatch.setattr(
326+
blosc2.FsspecNDSource,
327+
"get_chunk",
328+
lambda self, nchunk: (fetched.append(nchunk), orig(self, nchunk))[1],
329+
)
330+
318331
cache = asyncio.run(p.afetch(slice(150, 250)))
319332
assert np.array_equal(cache[150:250], a[150:250])
333+
assert fetched == [1, 2]
320334

321335

322336
def test_lazy_fetch_is_serial_when_asked(monkeypatch):

0 commit comments

Comments
 (0)