fix(streaming): _S3SeekableIO.__next__/__iter__ don't advance self._position, corrupting subsequent seek()/read() - #8389
Conversation
…osition, corrupting subsequent seek()/read() __next__ and __iter__ delegated straight to raw_stream.__next__()/raw_stream.__iter__(), bypassing self._position bookkeeping entirely -- unlike read()/readline()/readlines(), which all correctly advance self._position by the number of bytes actually consumed. seek() uses self._position as ground truth to compute the S3 GetObject Range header on the next raw_stream access. Consuming any data via the iterator protocol (e.g. "for line in s3_object: ...", a fully supported documented usage) left self._position stuck at its pre-iteration value. A subsequent seek()/read() would then reopen the S3 stream with a Range header computed from that stale position -- silently returning the wrong slice of the object (skipped or duplicated bytes), with no exception raised. Fix: route __next__ through the already position-tracked readline(), and have __iter__ return self, matching the standard Python file-iterator protocol.
|
leandrodamascena
left a comment
There was a problem hiding this comment.
Thanks for catching the position tracking issue. The bug is real, but I think the fix needs one adjustment before we merge it.
StreamingBody iteration returns 1 KiB chunks, not lines. Routing __next__() through readline() changes the existing behavior. I reproduced this with the same payload:
- current behavior:
[1024, 481] - this PR:
[2, 1501, 2]
For an object without newlines, next() can also read the entire remaining object instead of a 1 KiB chunk.
Could we preserve the current iteration contract and only add the missing position tracking?
def __next__(self):
chunk = next(self.raw_stream)
self._position += len(chunk)
return chunk
def __iter__(self):
return selfThe tests should also use a payload larger than 1 KiB, confirm the chunk size remains unchanged, and test a partial iteration followed by SEEK_CUR and an actual read(). The current seek test consumes the whole object and then seeks beyond EOF, so it verifies the generated range but not the returned data.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #8389 +/- ##
===========================================
+ Coverage 96.64% 96.67% +0.03%
===========================================
Files 296 296
Lines 14765 14768 +3
Branches 1245 1246 +1
===========================================
+ Hits 14269 14277 +8
+ Misses 361 358 -3
+ Partials 135 133 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|



Issue number
Fixes #8388
Summary
Fixes a bug where iterating an
_S3SeekableIO/S3Objectstream (for line in s3_object: ...ornext(s3_object)) leavesself._positionstuck at its pre-iteration value, so a subsequentseek()computes the wrong S3 byte range and the followingread()silently returns the wrong slice of the object.Changes
__next__and__iter__previously delegated straight toraw_stream.__next__()/raw_stream.__iter__():Unlike
read()/readline()/readlines()(which all advanceself._positionby the number of bytes actually consumed), this bypassed position bookkeeping entirely.seek()usesself._positionas ground truth to build theRangeheader on the nextraw_streamaccess, so anyseek()after iterating would compute an offset relative to a stale position.Fix: route
__next__through the already position-trackedreadline(), and have__iter__returnself, matching the standard Python file-iterator protocol:This is functionally equivalent for consumers that only care about line-by-line iteration (the common case), while keeping position tracking correct for anyone who seeks after iterating.
User experience
Before this fix, code like:
would silently read the wrong bytes after the seek, with no exception raised. After this fix, position tracking stays correct across iteration, so
seek()/read()behave correctly regardless of whether the stream was previously consumed via.read()or via iteration.Checklist
developbranch docs, if applicable — N/A, internal bug fix, no public API/behavior change for correctly-functioning code pathsIs this a breaking change?
NO.
__iter__returningselfinstead ofraw_stream's iterator, and__next__routing throughreadline(), are both drop-in-compatible with the existing documented iteration usage (for line in s3_object,next(s3_object)) — only the previously-broken position tracking changes.Tests
Added to
tests/functional/streaming/_boto3/test_s3_seekable_io.py:test_next_advances_position— verifiestell()correctly reflects bytes consumed vianext()test_iter_returns_self— standard iterator protocol checktest_seek_after_iteration_uses_correct_range— reproduces the real failure mode: iterate the stream, then seek, and verify the resultingRangeheader reflects true consumed position rather than the stale pre-iteration valueAll three fail against the pre-fix code (verified by reverting the fix locally and re-running; the seek test shows the exact bug live: expected
bytes=28-, receivedbytes=5-) and pass after the fix.Full suite
tests/functional/streaming/_boto3/test_s3_seekable_io.py: 24 passed (independently re-verified).By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.