Files
windmill/python-client/wmill/wmill/s3_reader.py
T
Ruben Fiszel 13b521651b fix(python-client): return at most size bytes from S3BufferedReader.read (#10623)
* feat: add unit tests for S3BufferedReader.read and improve read method implementation

* feat: refactor S3BufferedReader.read method and add unit tests for its functionality

* feat: implement peek() on S3BufferedReader with buffered reads

* fix(python-client): keep the read(size) contract and trim the test surface

Drop the duplicated `TestS3BufferedReaderRead` class from
`python-client/tests/wmill_client_test.py`: CI runs `pytest tests/` from
`python-client/wmill`, so that legacy manual harness never executes, and the
same assertions already live in `python-client/wmill/tests/test_s3_reader.py`.

Narrow that file to the four behaviours a future change could break, and make
the `bytes_generator` guard actually call `bytes_generator`.

Align `peek()` with `io.BufferedReader.peek`, which does at most one read on
the underlying stream, rather than looping until `size` bytes are buffered.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(python-client): hold read1 to one underlying read

read1 forwarded to read, so read1(-1) drained the whole object — the same
unbounded buffering this branch removes from read. Now that a buffer exists,
read1 can honour its own contract: fill only when the buffer is empty, then
serve from it.

Also treat read(None) as read(-1), per the BufferedReader contract, and pin
that read(0) does not pull from the stream: that holds only because the
drain sentinel is a negative size, and widening it to any falsy size would
reintroduce whole-file buffering.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(python-client): return from read1(0) without touching the stream

A zero-length read has nothing to serve, so pulling a chunk to satisfy it
both wastes a round trip and advances the stream. Guard it ahead of the
fill, and pin it with a chunk source that counts pulls.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Tushar <tusharanshu18@gmail.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-10 23:04:27 +02:00

116 lines
3.7 KiB
Python

from io import BufferedReader, BytesIO
from typing import Optional, Union
import httpx
class S3BufferedReader(BufferedReader):
"""Streaming buffered reader for S3 files via Windmill's S3 proxy.
Args:
workspace: Windmill workspace ID
windmill_client: HTTP client for Windmill API
file_key: S3 file key/path
s3_resource_path: Optional path to S3 resource configuration
storage: Optional storage backend identifier
"""
def __init__(self, workspace: str, windmill_client: httpx.Client, file_key: str, s3_resource_path: Optional[str], storage: Optional[str]):
params = {
"file_key": file_key,
}
if s3_resource_path is not None:
params["s3_resource_path"] = s3_resource_path
if storage is not None:
params["storage"] = storage
self._context_manager = windmill_client.stream(
"GET",
f"/w/{workspace}/job_helpers/download_s3_file",
params=params,
timeout=None,
)
self._buffer = bytearray()
def __enter__(self):
reader = self._context_manager.__enter__()
if reader.status_code >= 400:
error_bytes = reader.read()
try:
error_text = error_bytes.decode('utf-8')
except UnicodeDecodeError:
error_text = str(error_bytes)
raise httpx.HTTPStatusError(
f"Failed to load S3 file: {reader.status_code} {reader.reason_phrase} - {error_text}",
request=reader.request,
response=reader
)
self._iterator = reader.iter_bytes()
return self
def peek(self, size=0):
"""Return buffered bytes without consuming them.
Reads the underlying stream at most once, so the amount returned may be
more or less than `size`.
"""
if not self._buffer:
self._fill(1)
return bytes(self._buffer)
def _fill(self, limit):
# iter_bytes() yields whole HTTP chunks (~64KB), so a caller asking for
# `limit` bytes has to accumulate until the buffer holds that many.
# A negative limit means drain the stream.
while limit < 0 or len(self._buffer) < limit:
try:
self._buffer.extend(next(self._iterator))
except StopIteration:
break
def read(self, size=-1):
# BufferedReader.read(None) is documented as equivalent to read(-1).
if size is None:
size = -1
self._fill(size)
if size < 0:
result = bytes(self._buffer)
self._buffer.clear()
return result
result = bytes(self._buffer[:size])
del self._buffer[:size]
return result
def read1(self, size=-1):
"""Return up to `size` bytes, reading the underlying stream at most once.
Unlike `read`, a negative `size` returns only what is already buffered
rather than draining the whole object.
"""
if size == 0:
return b""
if not self._buffer:
self._fill(1)
if size is None or size < 0:
size = len(self._buffer)
result = bytes(self._buffer[:size])
del self._buffer[:size]
return result
def __exit__(self, *args):
self._context_manager.__exit__(*args)
def bytes_generator(buffered_reader: Union[BufferedReader, BytesIO]):
"""Yield 50KB chunks from a buffered reader.
Args:
buffered_reader: File-like object to read from
Yields:
Bytes chunks of up to 50KB
"""
while True:
byte = buffered_reader.read(50 * 1024)
if not byte:
break
yield byte