Skip to content

iostream: Deprecate read_until_close and reimplement as a loop - #3760

Open
mokashang wants to merge 1 commit into
tornadoweb:masterfrom
mokashang:refactor/read-until-close-partial-loop
Open

mokashang wants to merge 1 commit into
tornadoweb:masterfrom
mokashang:refactor/read-until-close-partial-loop

Conversation

@mokashang

Copy link
Copy Markdown
Contributor

Fixes #3728.

BaseIOStream.read_until_close had its own dedicated code path through
BaseIOStream: a _read_until_close flag was set when the method was
called, special-cased in close() to deliver the buffered data (or let
_signal_closed fail with StreamClosedError(real_error=…)), and
coordinated with the internal _read_buffer as the data accumulated
there until the stream closed. The docstring already pointed callers
at the same loop a user can write themselves — read_bytes with
partial=True, iterated until a StreamClosedError with real_error=None.

This PR removes the special case and makes the method just that loop:

  • read_until_close() kicks off the first read_bytes(..., partial=True)
    synchronously so an inline error (closed stream whose read buffer is
    empty, or a test that patches _try_inline_read) still raises from
    the call itself, matching the historical behavior
    (test_read_until_close_with_error).
  • _read_until_close_loop() is a small async def that awaits the
    pending read, accumulates the chunk, requests the next partial read,
    and treats StreamClosedError with real_error=None or a
    connection reset as a normal close (returning the accumulated bytes)
    — preserving test_read_until_close_after_close,
    test_read_until_close_after_error_close, and
    test_read_until_close_after_connection_reset.
  • Overflow is detected when len(buf) passes max_buffer_size and
    raised as StreamClosedError(real_error=StreamBufferFullError) with
    the same "Reached maximum read buffer size" log line used by
    _read_to_buffer. The old "error on read" log line from
    _handle_read is no longer emitted for this specific case because
    the overflow is now detected at the user-buffer level rather than
    inside the read handler; test_read_until_close_with_buffer_overflow
    is updated accordingly.
  • The internal _read_until_close attribute and its branch in
    close() are deleted; the stale read_until_close mention in the
    _read_to_buffer_loop target-bytes comment is updated.

A new regression test (test_read_until_close_chunked) covers the
loop's reassembly path by driving max_buffer_size bytes through
read_until_close one byte at a time with intervening checkpoints,
ensuring the result fills up to (and no further than) the limit.

The method's docstring is now also tagged with .. deprecated:: 6.6
and a .. versionchanged:: 6.6 note explaining both the deprecation
reasoning (finer flow control, clearer error reporting after a partial
response) and the under-the-hood refactor.

Testing

  • python3 -m tornado.test passes (1390 tests, 79 skipped).
  • python3 -m unittest tornado.test.iostream_test.TestIOStream tornado.test.iostream_test.TestIOStreamSSL tornado.test.iostream_test.TestIOStreamSSLContext passes 108/108
    including every test_read_until_close_* case.
  • black --check and mypy are clean on the changed files; flake8
    has no new warnings.

The changelog entry has been added under docs/releases/v6.6.0.rst
under both "Deprecation notices" and the new tornado.iostream
section.

Replace the dedicated BaseIOStream.read_until_close code path with a
loop that calls read_bytes with partial=True, as recommended for callers
in the method's own docstring. The externally observable behavior is
unchanged: connection-reset-as-normal-close, StreamClosedError with
real_error for closes with exc_info, inline propagation of errors from
_try_inline_read, and StreamBufferFullError on buffer overflow are all
preserved and exercised by the existing tests.

The internal _read_until_close flag and its special case in close() are
removed, and the stale read_until_close reference in the
_read_to_buffer_loop target-bytes comment is updated. A new test
confirms the loop reassembles chunks that arrive one byte at a time.

The method is now also marked deprecated in its docstring, encouraging
new callers to open-code the same loop for finer flow control and
clearer error reporting after a partial response.

Fixes tornadoweb#3728

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Soft-deprecate and reimplement IOStream.read_until_close

1 participant