From a38fd2cb11257f70dbaf7408ae04a9857ee95fe6 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Tue, 22 Sep 2026 13:31:02 -0400 Subject: [PATCH] fix(corekit): keep SeekableReader's position bookkeeping in step (#643, #644) - seek assigned _tell before validating it, so a refused seek left the position at the rejected offset and the next read returned the wrong octet. The target is now validated before it is committed. - SEEK_CUR and SEEK_END never checked the resulting position, so a negative absolute _tell was reachable. The check now applies under every whence, and with buffer_save=True, where the old refusal was skipped entirely and seek returned the negative position as though it had worked. - peek read through self._buffer and advanced its cursor while leaving _tell alone, so the next buffered read started from the wrong octet. It now reads through the view and leaves the cursor where it found it. - readline, read1 and peek capped size with min(size, _buffer_cur - 1): a count less one, measured from the window's base rather than the position read from, which handed back NUL padding at a plausible length and so suppressed the top-up from the stream. All three now derive the count from the position, as read already did. - A position the window has dropped raises rather than being answered from the buffer's stale cursor -- except for a zero-length read, which wants no octets and so is answered without consulting the window at all. tests/corekit/test_io.py: 31 passed, 35 subtests, 100% coverage of the module. 3000 random operation sequences: 6729 inconsistent states and 446 undocumented exceptions before, zero of each after. --- pcapkit/corekit/io.py | 139 +++++++++++++-- tests/corekit/test_io.py | 373 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 493 insertions(+), 19 deletions(-) diff --git a/pcapkit/corekit/io.py b/pcapkit/corekit/io.py index 1ae8cc3bb..2dfbb3347 100644 --- a/pcapkit/corekit/io.py +++ b/pcapkit/corekit/io.py @@ -147,6 +147,46 @@ def _write_buffer(self, buf: 'bytes', /) -> 'None': else: self._buffer_view[old_ptr:self._buffer_cur] = buf + def _seek_buffer(self) -> 'int': + """Point the buffer at the current stream position, and say how much it can serve. + + The buffer is a sliding window whose octet 0 sits at absolute ``_buffer_set`` and + whose content occupies ``[0:_buffer_cur]``, so ``_buffer_set + _buffer_cur`` is how + far the underlying stream has been consumed. Two things follow, and every buffered + read path needs both of them: + + * The buffer's own cursor is a **derived** quantity, ``_tell - _buffer_set``, rather + than a fourth piece of state to be maintained. Nothing was maintaining it: a read + served from the stream writes through :attr:`_buffer_view` and leaves the cursor + untouched, :meth:`_write_buffer` rewinds it to the start of the appended octets + when the window slides, and :meth:`peek` used to advance it while leaving ``_tell`` + alone. Deriving it here, at each point of use, is what makes the drift + unrepresentable instead of merely repaired afterwards by the next :meth:`seek`. + * What is available is the run from the position to the end of the content, + ``_buffer_set + _buffer_cur - _tell`` -- not ``_buffer_cur``, which is measured + from the window's base and so counts octets that lie *behind* the position, and + not that count less one, which is short of both. Over-asking does not fail: the + buffer is allocated full of NUL padding, so it hands that back as though it were + data, with a plausible length that then suppresses the top-up from the stream. + + Returns: + The number of octets the buffer can answer for from the current position. + + Raises: + SeekError: If the position lies before the window, whose octets are then gone + for good -- the stream cannot be rewound to re-supply them. :meth:`seek` + refuses that position for the same reason; it is reachable here only through + a :meth:`truncate` that moved the window's base past a position already set. + + """ + buf_off = self._tell - self._buffer_set + if buf_off < 0: + raise SeekError(f'cannot read before the beginning of the buffer: ' + f'{self._tell} < {self._buffer_set}') + + self._buffer.seek(buf_off, io.SEEK_SET) + return self._buffer_cur - buf_off + def close(self) -> 'None': """Flush and close this stream. This method has no effect if the file is already closed. Once the file is closed, any operation on the file (e.g. reading or writing) will raise @@ -211,10 +251,29 @@ def readline(self, size: 'int | None' = -1, /) -> 'bytes': with open(self._buffer_path, 'rb') as temp_file: temp_file.seek(self._tell, io.SEEK_SET) buf = temp_file.readline(size) + elif not size: + # NOTE: a request for no octets is answered without consulting the window, + # which has nothing to say about it. Asking anyway refuses a zero-length + # read from a position :meth:`truncate` has left behind the window -- a + # refusal over data that was never wanted, and not what this used to do. + # + # Truthiness rather than ``size == 0`` on purpose. It is the same test for + # every value this method can be reached with -- ``size`` is an ``int`` by + # the time control arrives, and ``False`` is ``0`` -- but :meth:`peek` does + # not normalise a ``None`` its signature does not permit, and there the two + # spellings diverge: ``size == 0`` would send ``None`` on to the window and + # report a type error as a position error. Keeping it falsy keeps that call + # failing as the :exc:`TypeError` it always was. + buf = b'' else: - buf = self._buffer.readline(min(size, self._buffer_cur - 1)) + buf_rem = self._seek_buffer() + buf = self._buffer.readline(buf_rem if size < 0 else min(size, buf_rem)) - if not buf.endswith(b'\n') and (size_rem := size - len(buf)) > 0: + # NOTE: an unbounded ``readline`` has to keep going until the line ends, and + # capping it at the buffer's content -- which the line need not end inside -- + # is what makes the continuation necessary rather than optional here. + size_rem = -1 + if not buf.endswith(b'\n') and (size < 0 or (size_rem := size - len(buf)) > 0): buf_tmp = self._stream.readline(size_rem) self._write_buffer(buf_tmp) buf += buf_tmp @@ -264,6 +323,23 @@ def seek(self, offset: 'int', whence: 'int' = io.SEEK_SET, /) -> 'int': Return the new absolute position. + Note: + The target is computed, then validated, and only then written to ``_tell``. A + branch that assigned the position before deciding whether to accept it left a + refused seek having moved it anyway, with the resync below -- the one thing that + puts the buffer's cursor back in step -- skipped on the way out. A caller that + catches the error and reasonably takes the position to be unchanged then read + from the rejected offset instead, silently and without a second error. + + The negative check applies to every ``whence``, rather than only + :data:`~io.SEEK_SET` looking at its offset. An absolute position below zero + cannot exist under any of them, and it is not the same failure as a position + that has merely slid out of the window -- which is ordinary, and recoverable + with ``buffer_save=True``. Reporting the first as ``negative seek value`` keeps + the two distinguishable, and being a property of the position rather than of the + window it holds with a saved buffer as well, where the old refusal did not + apply at all and the seek returned a negative position as if it had worked. + """ # NOTE: we mark the end of buffer content to the end of buffer # so that it may trigger the IO to read more data to fill in @@ -272,16 +348,23 @@ def seek(self, offset: 'int', whence: 'int' = io.SEEK_SET, /) -> 'int': #buf_end = self._buffer_set + self._buffer_cur if whence == io.SEEK_SET: - if offset < 0: - raise SeekError(f'negative seek value {offset}') - self._tell = offset + target = offset elif whence == io.SEEK_CUR: - self._tell += offset + target = self._tell + offset elif whence == io.SEEK_END: - self._tell = buf_end + offset + target = buf_end + offset else: raise SeekError(f'invalid whence ({whence}, should be {io.SEEK_SET}, {io.SEEK_CUR} or {io.SEEK_END})') + if target < 0: + raise SeekError(f'negative seek value {target}') + # NOTE: both refusals read the target rather than ``_tell``, which is what lets them + # run before the position is committed. This one is the window's, so a saved buffer + # -- which can still supply the octets from file -- is exempt from it. + if target < self._buffer_set and self._buffer_file is None: + raise SeekError(f'cannot seek before the beginning of the buffer: {target} < {self._buffer_set}') + + self._tell = target if self._tell >= self._buffer_set: if self._tell > buf_end: warn(f'seek beyond the end of the buffer: {self._tell} > {buf_end}', @@ -300,8 +383,10 @@ def seek(self, offset: 'int', whence: 'int' = io.SEEK_SET, /) -> 'int': self._tell = tmp_end + len(tmp_buf) self._buffer.seek(self._tell - self._buffer_set, io.SEEK_SET) else: - if self._buffer_file is None: - raise SeekError(f'cannot seek before the beginning of the buffer: {self._tell} < {self._buffer_set}') + # NOTE: only a saved buffer reaches here -- the refusal above has already + # turned away a position before the window when there is no file to serve it + # from, which is what the refusal used to do at this point instead, after + # ``_tell`` had been moved. self._buffer.seek(0, io.SEEK_SET) return self._tell @@ -414,15 +499,11 @@ def read(self, size: 'int | None' = -1, /) -> 'bytes': with open(self._buffer_path, 'rb') as temp_file: temp_file.seek(self._tell, io.SEEK_SET) buf = temp_file.read(size) + elif not size: + # NOTE: as in :meth:`readline` -- no octets wanted, so no window check. + buf = b'' else: - # NOTE: ``_buffer_cur`` counts the octets written into the buffer, so what - # is available from here is the run between the current position and the end - # of that content. That count less one is neither: at the start of the - # buffer it is one octet short, and the shortfall is then made up from the - # stream -- past the octet that was skipped, losing it -- while further in - # it reaches beyond the content and hands the padding behind it back as - # data, which is also what an uncapped read does. - buf_rem = self._buffer_set + self._buffer_cur - self._tell + buf_rem = self._seek_buffer() buf = self._buffer.read(buf_rem if size < 0 else min(size, buf_rem)) size_rem = -1 @@ -452,8 +533,14 @@ def read1(self, size: 'int | None' = -1, /) -> 'bytes': with open(self._buffer_path, 'rb') as temp_file: temp_file.seek(self._tell, io.SEEK_SET) buf = temp_file.read1(size) + elif not size: + # NOTE: as in :meth:`readline` -- no octets wanted, so no window check. + buf = b'' else: - buf = self._buffer.read1(min(size, self._buffer_cur - 1)) + # NOTE: no continuation from the stream when the buffer answered, since + # :meth:`read1` is specified to return only buffered octets if any are. + buf_rem = self._seek_buffer() + buf = self._buffer.read1(buf_rem if size < 0 else min(size, buf_rem)) if not buf: # pragma: no cover size_rem = -1 @@ -539,8 +626,22 @@ def peek(self, size: 'int' = 0) -> 'bytes': with open(self._buffer_path, 'rb') as temp_file: temp_file.seek(self._tell, io.SEEK_SET) buf = temp_file.peek(size) + elif not size: + # NOTE: as in :meth:`readline` -- no octets wanted, so no window check. Only + # the buffered branch is short-circuited: the branch above hands a bare + # ``peek()`` to the raw stream, whose own ``peek(0)`` may legitimately + # return a whole buffer's worth, and that is left as it was. + buf = b'' else: - buf = self._buffer.read(min(size, self._buffer_cur - 1)) + # NOTE: read through the view rather than through ``self._buffer``, whose + # cursor an ordinary read would advance. A preview must leave the position + # alone, and the buffer's cursor is part of the position: advancing it here + # while leaving ``_tell`` untouched is what made the *next* buffered read + # start from the wrong octet, with ``tell()`` reporting the right one. + buf_rem = self._seek_buffer() + buf_off = self._buffer.tell() + buf = bytes(self._buffer_view[ + buf_off:buf_off + (buf_rem if size < 0 else min(size, buf_rem))]) if not buf and len(buf) < size: # pragma: no cover size_rem = -1 diff --git a/tests/corekit/test_io.py b/tests/corekit/test_io.py index 264ac5937..e04d37081 100644 --- a/tests/corekit/test_io.py +++ b/tests/corekit/test_io.py @@ -445,6 +445,379 @@ def test_saved_readline_and_empty_buffer_refill_edges(self) -> None: self.assertEqual(reader.readlines(5), []) self._close_reader(reader) + # ------------------------------------------------------------------ # + # issue #643 -- position bookkeeping across operations + # ------------------------------------------------------------------ # + + def _pipe_stream(self, data: bytes): + """A non-seekable stream with neither ``peek`` nor ``read1`` -- i.e. a pipe. + + ``SeekableReader``'s only production consumer wraps exactly this kind of stream + (``pcapkit/foundation/extraction.py``, where a non-seekable input is wrapped and + then peeked), and the branches it selects differ from the ones a + :class:`io.BytesIO` selects: with no ``peek`` of its own, :meth:`peek` has to do a + real consuming read on it and stash the result in the buffer. + + """ + class Pipe: + def __init__(self, payload: bytes) -> None: + self._stream = io.BytesIO(payload) + self.consumed = 0 + + def read(self, size=-1): + buf = self._stream.read(size) + self.consumed += len(buf) + return buf + + def readline(self, size=-1): + buf = self._stream.readline(size) + self.consumed += len(buf) + return buf + + def readable(self): + return True + + def seekable(self): + return False + + def flush(self): + return None + + def isatty(self): + return False + + def close(self): + return self._stream.close() + + return Pipe(data) + + def test_a_refused_seek_leaves_the_position_untouched(self) -> None: + """A ``seek`` that raises must not have moved the position on its way out. + + Each ``whence`` branch assigned ``_tell`` before anything validated the result, so a + refusal left the position at the rejected target with the resync at the end of + :meth:`seek` -- the one thing that puts the buffer's cursor back in step -- skipped. + A caller that catches :exc:`SeekError` and reasonably takes the position to be + unchanged then read from the rejected offset instead, silently and with no second + error to show for it. + + The octet asserted after the refusal is what makes this discriminating: + ``test_seek_before_buffer_start_requires_saved_buffer`` above drives the identical + path but stops at ``assertRaises``, and the whole defect lives in what happens next. + Two buffer sizes, so neither result can be an artefact of one window geometry. + + """ + reader = self.SeekableReader(io.BytesIO(b'ABCDEFGHIJKLMNOPQRSTUVWXYZ'), buffer_size=2) + self.assertEqual(reader.read(13), b'ABCDEFGHIJKLM') + with self.assertRaises(self.exceptions.SeekError): + reader.seek(8) + self.assertEqual(reader.tell(), 13) # was 8, the rejected target + self.assertEqual(reader.read(1), b'N') # was b'L', the octet at 11 + self._close_reader(reader) + + reader = self.SeekableReader(io.BytesIO(b'ABCDEFGHIJKLMNOPQRSTUVWXYZ'), buffer_size=4) + self.assertEqual(reader.read(6), b'ABCDEF') + with self.assertRaises(self.exceptions.SeekError): + reader.seek(0) + self.assertEqual(reader.tell(), 6) # was 0 + self.assertEqual(reader.read(1), b'G') # was b'C', the octet at 2 + self._close_reader(reader) + + def test_seek_refuses_a_negative_absolute_position_under_every_whence(self) -> None: + """``SEEK_CUR`` and ``SEEK_END`` did arithmetic and never looked at the result. + + Only ``SEEK_SET`` checked its offset, so an absolute position below zero was + reachable through the other two: ``tell()`` came back ``-100`` and ``-96``. The + asymmetry is the defect -- the same impossible request was a clean ``negative seek + value`` under one ``whence`` and a misleading ``cannot seek before the beginning of + the buffer`` under the others, which conflates a position that cannot exist with one + that has merely slid out of the window, the latter being ordinary and recoverable + with ``buffer_save=True``. + + The offsets are large enough to cross zero, which is what the existing + ``seek(-1, io.SEEK_CUR)`` assertions do not do. + + """ + for whence, offset, message in [ + (io.SEEK_SET, -1, 'negative seek value -1'), + (io.SEEK_CUR, -100, 'negative seek value -100'), + (io.SEEK_END, -100, 'negative seek value -96'), # buf_end is 4 + ]: + with self.subTest(whence=whence, offset=offset): + reader = self.SeekableReader(io.BytesIO(b'abcdef'), buffer_size=4) + with self.assertRaises(self.exceptions.SeekError) as ctx: + reader.seek(offset, whence) + self.assertEqual(str(ctx.exception), message) + self.assertEqual(reader.tell(), 0) + self._close_reader(reader) + + def test_seek_refuses_a_negative_position_with_a_saved_buffer_too(self) -> None: + """With ``buffer_save=True`` the refusal was skipped and the seek reported success. + + The window refusal is conditional on there being no buffer file, since a saved + buffer really can supply octets from before the window. A negative absolute position + is not that case, and with a file the old code fell straight through and *returned* + ``-5`` as though the seek had worked. The failure then surfaced on an unrelated call + as a bare ``OSError: [Errno 22] Invalid argument`` out of the middle of + :meth:`read`, from a line only reachable because :meth:`seek` had accepted something + it should not have. + + """ + with tempfile.NamedTemporaryFile(delete=False) as temp: + path = temp.name + try: + reader = self.SeekableReader(io.BytesIO(b'abcdefghij'), buffer_size=4, + buffer_save=True, buffer_path=path) + with self.assertRaises(self.exceptions.SeekError) as ctx: + reader.seek(-5, io.SEEK_CUR) # returned -5, no exception at all + self.assertEqual(str(ctx.exception), 'negative seek value -5') + self.assertEqual(reader.tell(), 0) + self.assertEqual(reader.read(1), b'a') # was OSError: [Errno 22] + self._close_reader(reader) + finally: + if os.path.exists(path): + os.unlink(path) + + def test_peek_does_not_move_what_the_next_read_returns(self) -> None: + """A preview must leave the position alone, and the buffer's cursor is part of it. + + :meth:`peek` correctly never touched ``_tell``, but its buffered branch read + *through* ``self._buffer``, advancing that :class:`io.BytesIO`'s own cursor. Nothing + put it back, so ``_tell`` was right, the cursor was wrong, and the next buffered + read started from the wrong octet -- while ``tell()`` reported the same number in + both runs, leaving the caller no way to tell them apart. Each case here therefore + asserts against a **control run** that omits only the ``peek``. + + Both branches are covered, because they drift by different routes. The + :class:`io.BytesIO` case takes the buffered branch. The pipe case takes the other + one: with no ``peek`` of its own the stream is read for real and the result handed to + ``_write_buffer``, which writes through the memoryview and so leaves the cursor + wherever it already was -- five octets adrift, replaying the first octets ever + buffered. + + The data starts at ``\\x01`` rather than ``\\x00`` deliberately: ``\\x00`` is exactly + the buffer's NUL padding, so data containing it cannot distinguish a real octet from + a padded one. + + """ + data = bytes(range(1, 51)) + + reader = self.SeekableReader(io.BytesIO(data), buffer_size=16) + reader.read(8) + reader.seek(2) + control = reader.read(3) + self.assertEqual(control, b'\x03\x04\x05') + self._close_reader(reader) + + reader = self.SeekableReader(io.BytesIO(data), buffer_size=16) + reader.read(8) + reader.seek(2) + self.assertEqual(reader.peek(3), b'\x03\x04\x05') + self.assertEqual(reader.tell(), 2) + # the cursor is the position's other half, and a preview may not have moved it + self.assertEqual(reader._buffer.tell(), reader._tell - reader._buffer_set) + self.assertEqual(reader.read(3), control) # was b'\x06\x07\x08' + self._close_reader(reader) + + stream = self._pipe_stream(data) + reader = self.SeekableReader(stream, buffer_size=16, stream_closing=False) + reader.read(8) + control = reader.read(4) + self.assertEqual(control, b'\x09\x0a\x0b\x0c') + self._close_reader(reader) + + stream = self._pipe_stream(data) + reader = self.SeekableReader(stream, buffer_size=16, stream_closing=False) + reader.read(8) + self.assertEqual(reader.peek(4), b'\x09\x0a\x0b\x0c') + self.assertEqual(reader.tell(), 8) + self.assertEqual(reader.read(4), control) # was b'\x01\x02\x03\x04' + self._close_reader(reader) + + # ------------------------------------------------------------------ # + # issue #644 -- the count-less-one measured from the wrong origin + # ------------------------------------------------------------------ # + + def test_buffered_reads_return_every_octet_available_from_the_position(self) -> None: + """``min(size, _buffer_cur - 1)`` is wrong twice, and both halves are asserted here. + + ``_buffer_cur`` counts from the window's base, not from the position being read + from, so it offers octets that lie *behind* the position; and the ``- 1`` is short of + even that. What is genuinely available is + ``_buffer_set + _buffer_cur - _tell``, which is what each path now asks for. + + The two halves fail differently, so each gets its own case: + + * The ``- 1`` alone, isolated by seeking to the window's base. The cap is one short, + the shortfall is made up from the raw stream -- which has already moved past the + octet that was skipped -- and the octet is dropped from the *middle* of the answer. + * The wrong origin, isolated by seeking strictly inside the buffered region. This is + the worse of the two: the buffer is allocated full of NUL padding, so over-asking + returns real octets followed by padding at the *correct length*, which makes + ``size_rem`` zero and skips the top-up entirely -- so the real octets are never + fetched at all. + + ``read`` already carried the corrected count before this change; ``readline``, + ``read1`` and ``peek`` are the three sites that did not. All four are asserted, the + first as a regression guard. + + """ + # the `- 1` alone: seek to the base, ask for more than the cap allowed + for method, data, size, expected in [ + ('read', b'abcde', 5, b'abcde'), # was b'abce' + ('readline', b'abcde\nfghij\n', 6, b'abcde\n'), # was b'abce\n' + ('read1', b'abcde', 4, b'abcd'), # was b'abc' + ('peek', b'abcde', 4, b'abcd'), # was b'abc' + ]: + with self.subTest(half='count-less-one', method=method): + reader = self.SeekableReader(io.BytesIO(data)) + self.assertEqual(reader.read(4), b'abcd') + self.assertEqual(reader.seek(0), 0) + self.assertEqual(getattr(reader, method)(size), expected) + self._close_reader(reader) + + # the wrong origin: seek strictly inside the buffered region, ask past its end + for method, data, size, expected in [ + ('read', b'0123456789' + b'X' * 20, 8, b'56789XXX'), # was b'56789\x00\x00\x00' + ('readline', b'0123456789\n' + b'X' * 20, 8, b'56789\n'), + ('read1', b'0123456789' + b'X' * 20, 8, b'56789'), + ('peek', b'0123456789' + b'X' * 20, 8, b'56789'), + ]: + with self.subTest(half='wrong-origin', method=method): + reader = self.SeekableReader(io.BytesIO(data)) + self.assertEqual(reader.read(10), b'0123456789') + self.assertEqual(reader.seek(5), 5) + got = getattr(reader, method)(size) + self.assertEqual(got, expected) + # NOTE: whatever the length, none of it may be the buffer's NUL padding -- + # that substitution is the defect, and the data has no NUL in it. + self.assertNotIn(b'\x00', got) + self._close_reader(reader) + + def test_an_unbounded_buffered_readline_still_finishes_the_line(self) -> None: + """Capping ``readline`` at the buffer must not truncate a line that runs past it. + + With no size the old cap was ``min(-1, _buffer_cur - 1) == -1``, i.e. uncapped, and + :meth:`io.BytesIO.readline` then ran off the content into the NUL padding behind it. + Capping at the content is right, but on its own it would leave an unbounded + ``readline`` stopping at the window's edge rather than at the newline, so the + continuation from the stream now runs for a negative size as well -- the same shape + :meth:`read` already had. + + """ + reader = self.SeekableReader(io.BytesIO(b'abcdefghij\nklm'), buffer_size=8) + self.assertEqual(reader.read(4), b'abcd') + self.assertEqual(reader.seek(0), 0) + self.assertEqual(reader.readline(), b'abcdefghij\n') + self.assertEqual(reader.tell(), 11) + self._close_reader(reader) + + # ------------------------------------------------------------------ # + # the invariant both issues turn on + # ------------------------------------------------------------------ # + + def test_the_window_base_and_content_pointer_track_the_stream_consumption(self) -> None: + """``_buffer_set + _buffer_cur`` is how far the stream has been read, always. + + This is the invariant #633's first revision broke, turning a loud + :exc:`ValueError` into silent data corruption: :meth:`seek` reads the pair as the + stream's consumption point and reads ahead from it to fill a gap, so a pair that + under-reports makes the next forward seek splice octets in from the wrong absolute + offset without complaining. It is asserted directly here, against a stream that + counts what has actually been taken off it, after *every* operation rather than at + the end -- an output assertion can pass while the bookkeeping behind it is already + wrong, which is exactly how that revision got as far as review. + + Deriving the buffer's cursor from ``_tell`` at each point of use is what preserves + the pair by construction: nothing clamps or adjusts either member to make a read fit, + so neither can drift from what ``_write_buffer`` recorded. + + """ + stream = self._pipe_stream(b'abcdefghijklmnop') + reader = self.SeekableReader(stream, buffer_size=8, stream_closing=False) + + operations = [ + ('read', lambda: reader.read(5), b'abcde'), + ('peek', lambda: reader.peek(3), b'fgh'), + ('read', lambda: reader.read(3), b'fgh'), + ('seek', lambda: reader.seek(2), 2), + ('readline', lambda: reader.readline(4), b'cdef'), + ('read1', lambda: reader.read1(2), b'gh'), + ('truncate', lambda: reader.truncate(4), 4), + ('read', lambda: reader.read(2), b'ij'), + ] + for name, operation, expected in operations: + with self.subTest(operation=name, expected=expected): + self.assertEqual(operation(), expected) + self.assertEqual(reader._buffer_set + reader._buffer_cur, stream.consumed) + self.assertGreaterEqual(reader._tell, 0) + self.assertGreaterEqual(reader._buffer_set, 0) + self.assertLessEqual(reader._buffer_cur, reader._buffer_size) + self.assertEqual(len(reader._buffer.getvalue()), reader._buffer_size) + self._close_reader(reader) + + def test_a_position_the_window_has_dropped_is_refused_not_guessed(self) -> None: + """A read from before the window raises, rather than answering from the wrong octet. + + :meth:`truncate` advances the window's base past the octets it drops, which can + leave a position already set sitting before it. :meth:`seek` refuses that position + outright -- the stream cannot be rewound to re-supply the octets, so there is + nothing to read -- but the buffered read paths reached it too, by way of a position + set *before* the truncation rather than after, and answered from whatever offset the + cursor happened to hold. Here that returned ``b'c'``, the octet at 2, for a read at + 1. + + Refusing it is the same answer :meth:`seek` already gives, and turns the last silent + wrong answer in this family into a loud one. + + """ + reader = self.SeekableReader(io.BytesIO(b'abcdefghijkl'), buffer_size=8) + + self.assertEqual(reader.read(4), b'abcd') + self.assertEqual(reader.seek(1), 1) + self.assertEqual(reader.truncate(2), 2) + self.assertEqual(reader._buffer_set, 2) # the window now starts at 2 + self.assertEqual(reader.tell(), 1) # and the position is behind it + + with self.assertRaises(self.exceptions.SeekError): + reader.read(1) # was b'c', the octet at 2 + with self.assertRaises(self.exceptions.SeekError): + reader.seek(1) + self._close_reader(reader) + + def test_a_zero_length_read_is_answered_without_consulting_the_window(self) -> None: + """A request for no octets must not be refused for having nowhere to read from. + + The refusal in the previous test is on the window, and the window has nothing to say + about a request that wants nothing: every one of these returned ``b''`` before the + refusal existed, and a zero-length read failing on position grounds is not a + behaviour either issue asked for. Checking the window before looking at the requested + size made all four of them raise from a position :meth:`truncate` had stranded. + + The same position is asserted both ways round, which is what makes this a statement + about the *size* rather than about the state: at size zero all four return, and the + test above shows ``read(1)`` from that identical state still raising. + + Only the buffered branch is short-circuited. A bare ``peek()`` on a position at or + past the buffered content still goes to the raw stream, whose own ``peek(0)`` may + return a whole buffer's worth, so the healthy cases here pin that down too. + + """ + for stranded in (False, True): + for method, args in [('read', (0,)), ('read1', (0,)), ('readline', (0,)), + ('peek', (0,)), ('peek', ())]: + with self.subTest(stranded=stranded, method=method, args=args): + reader = self.SeekableReader(io.BytesIO(b'abcdefghijkl'), buffer_size=8) + self.assertEqual(reader.read(4), b'abcd') + self.assertEqual(reader.seek(1), 1) + if stranded: + self.assertEqual(reader.truncate(2), 2) + self.assertEqual(reader._buffer_set, 2) + self.assertLess(reader.tell(), reader._buffer_set) + + self.assertEqual(getattr(reader, method)(*args), b'') + self.assertEqual(reader.tell(), 1) # and nothing moved + self._close_reader(reader) if __name__ == '__main__':