Rewind file bodies when internally retrying a request - #13330
Conversation
psf-chronographer
Bot
added
the
bot:chronographer:provided
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #13330 +/- ##
========================================
Coverage 98.98% 98.98%
========================================
Files 132 132
Lines 49073 49174 +101
Branches 2553 2557 +4
========================================
+ Hits 48576 48677 +101
Misses 373 373
Partials 124 124
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
Merging this PR will improve performance by 19.82%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | test_client_send_large_websocket_compressed_messages[isal.isal_zlib] |
86.4 ms | 72.1 ms | +19.82% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing aiolibsbot:koan/implement-13329 (785249e) with master (d9aaf69)2
Footnotes
-
83 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
-
No successful run was found on
master(6264834) during the generation of this report, so d9aaf69 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
|
The fix looks right. extending it to the case where the request is retried should work. Just didn't plan for that case |
Retry path now mirrors 307/308 path: reuses |
|
|
Rebase with requested adjustmentsBranch Changes applied
Not changed (and why)
StatsActions performed
CI statusCI will be checked asynchronously. Automated by Kōan |
530625f to
2a7cf1a
Compare
|
|
Rebase with requested adjustmentsBranch Changes applied
Not changed (and why)
StatsActions performed
CI statusCI will be checked asynchronously. Automated by Kōan |
|
|
Rebase with requested adjustmentsBranch Changes applied
Not changed (and why)
StatsActions performed
CI statusCI will be checked asynchronously. Automated by Kōan |
|
|
Rebase with requested adjustmentsBranch Changes applied
Not changed (and why)
StatsActions performed
CI statusCI will be checked asynchronously. Automated by Kōan |
| Fixed internally retried requests sending a truncated body when the request | ||
| data was a file object -- by :user:`aiolibsbot`. | ||
|
|
||
| The payload of the failed attempt is now reused on retry, so the file handle is | ||
| rewound to the position it had before the first attempt instead of resuming from | ||
| wherever the aborted write left it. Bodies that cannot be replayed (such as a | ||
| partially consumed async iterable) are no longer retried; the original | ||
| connection error is raised instead. A streamed body part read from an incoming | ||
| multipart request marks itself unreplayable once written, and a multipart body | ||
| reports that it cannot be replayed when any of its parts cannot, so neither the | ||
| retry nor the redirect path sends it again with a drained part missing. |
There was a problem hiding this comment.
Still open. Fragment now 11 lines, up from 8. Repo norm runs 1-4 lines: 12088.bugfix.rst 1 line, 12540.bugfix.rst 1 line, 10611.bugfix.rst 3, 12296.bugfix.rst 4. Keep sentence one plus one behaviour-change line. Drop payload-reuse and multipart mechanism — already in PR body. Flagged blocking this round.
|
|
Rebase with requested adjustmentsBranch Changes applied
Not changed (and why)
StatsActions performed
CI statusCI will be checked asynchronously. Automated by Kōan |
PR Review — Rewind file bodies when internally retrying a requestBoth outstanding maintainer requests are now addressed and the fix holds up under verification — merge-ready with nits. The core change is the right shape: reusing the failed attempt's payload instead of rebuilding it from The two
✅ Resolved since last review (1)Previously-flagged issues verified fixed
🟢 Suggestions
1. `await req._close()` can swap the retryable error for the writer task's exception
|
Rebase with requested adjustmentsBranch Changes applied
Not changed (and why)
StatsActions performed
CI statusCI will be checked asynchronously. Automated by Kōan |
|
Note No code changes since the last review ( |
Retrying a request after ClientOSError/ServerDisconnectedError rebuilt the payload from the original `data`. For a file object the handle had already been advanced by the aborted write, so the retry resent only the unread remainder with a matching (short) Content-Length. Reuse the payload from the failed attempt instead, as the redirect path already does: IOBasePayload seeks back to the position it recorded before the first attempt. Bodies that cannot be replayed are no longer retried; the original connection error propagates rather than a silently truncated body being sent. AsyncIterablePayload now marks itself consumed as soon as it starts draining an uncached iterator, so an interrupted write is correctly reported as unreplayable instead of only after StopAsyncIteration.
Rebase with requested adjustmentsBranch Changes applied
Not changed (and why)
StatsActions performed
CI statusCI will be checked asynchronously. Automated by Kōan |
ef3a00d to
785249e
Compare
bdraco
added
backport-3.14
2 participants
What do these changes do?
When
_requestinternally retries after aClientOSError/ServerDisconnectedError, itrebuilt the payload from the original
data. Ifdatawas an open file handle, theaborted write had already advanced it, so the retry resent only the unread remainder
(with a matching, short
Content-Length) — the server silently received a truncatedupload.
The retry path now reuses the payload from the failed attempt, exactly as the 307/308
redirect path already does.
IOBasePayload._set_or_restore_start_position()seeks thehandle back to the position recorded before the first attempt, so the full body is resent.
AsyncIterablePayloadadditionally marks itself consumed as soon as it starts draining anuncached iterator, rather than only on
StopAsyncIteration. Previously an interruptedwrite left
consumedasFalsewhile the iterator was partially drained, which made ahalf-sent body look replayable to both the retry and the redirect path.
Are there changes in behavior for the user?
Yes, two:
truncated one.
longer retried. The original connection error propagates instead of a silently truncated
body being sent.
Is it a substantial burden for the maintainers to support this?
No. It reuses the payload-replay mechanism the redirect path already relies on, rather
than adding a parallel one.
Related issue number
Fixes #13329
Checklist
retry path; behaviour change is described in the news fragment)
CONTRIBUTORS.txt— N/A(already a contributor)
CHANGES/folderAgent run output
New tests fail on
masterand pass with the fix:Full suite (pure Python):
The single failure is
tests/test_connector.py::test_tcp_connector_resolve_host, whichfails identically on
masterin this sandbox (no DNS).black,isort,flake8andmypyare clean on the changed files.Not verified with the C extensions:
pip install -e .fails to link in this environment.The change is confined to
client.py/payload.pyand touches no parser or websocket code.Drafted with Claude Code (Opus 5); reviewed by @bdraco.
Quality Report
Changes: 5 files changed, 97 insertions(+), 5 deletions(-)
Code scan: clean
Tests: failed (FAILED)
Branch hygiene: clean
Generated by Kōan