Add chunk tracing to .content by Dreamsorcerer · Pull Request #12889 · aio-libs/aiohttp

Dreamsorcerer · 2026-06-10T01:14:36Z

codecov · 2026-06-10T01:16:21Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.95%. Comparing base (ff38c23) to head (9ff2508).
⚠️ Report is 3 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #12889    +/-   ##
========================================
  Coverage   98.95%   98.95%            
========================================
  Files         131      131            
  Lines       47824    48043   +219     
  Branches     2480     2491    +11     
========================================
+ Hits        47324    47543   +219     
  Misses        376      376            
  Partials      124      124

Flag	Coverage Δ
Autobahn	`22.24% <12.94%> (-0.05%)`	⬇️
CI-GHA	`98.93% <100.00%> (+<0.01%)`	⬆️
OS-Linux	`98.69% <100.00%> (+<0.01%)`	⬆️
OS-Windows	`97.06% <100.00%> (+0.01%)`	⬆️
OS-macOS	`97.96% <100.00%> (+<0.01%)`	⬆️
Py-3.10	`98.18% <100.00%> (+<0.01%)`	⬆️
Py-3.11	`98.43% <100.00%> (+<0.01%)`	⬆️
Py-3.12	`98.52% <100.00%> (+<0.01%)`	⬆️
Py-3.13	`98.50% <100.00%> (+<0.01%)`	⬆️
Py-3.14	`98.51% <100.00%> (+<0.01%)`	⬆️
Py-3.14t	`97.60% <100.00%> (+<0.01%)`	⬆️
Py-pypy-3.11	`97.48% <100.00%> (+0.02%)`	⬆️
VM-macos	`97.96% <100.00%> (+<0.01%)`	⬆️
VM-ubuntu	`98.69% <100.00%> (+<0.01%)`	⬆️
VM-windows	`97.06% <100.00%> (+0.01%)`	⬆️
cython-coverage	`37.95% <11.76%> (-0.13%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

codspeed-hq · 2026-06-10T01:32:04Z

Merging this PR will not alter performance

✅ 79 untouched benchmarks
⏩ 79 skipped benchmarks¹

_{Comparing fix-tracing (9ff2508) with master (eb4d960)²}

79 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 (12ea5a5) during the generation of this report, so eb4d960 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

for more information, see https://pre-commit.ci

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

bdraco · 2026-06-13T02:41:49Z

-        if content and content.exception() is None:  # type: ignore[truthy-bool]
-            set_exception(content, _CONNECTION_CLOSED_EXCEPTION)
+        if content:  # type: ignore[truthy-bool]
+            if content.exception() is None:


need to check here if we could leak CancelledError

it didn't check before so likely fine but something to check now that I'm looking at it

No leak. except BaseException re-raises after close(); CancelledError still propagates. New aspect: cancelling a trace handler during a direct .content read now closes the response. Old read() path already closed on cancel. Confirm close-on-cancel acceptable for .content consumers.

bdraco · 2026-06-13T02:42:55Z

+            # read_nowait is sync but the hook is async; schedule it so the
+            # observability event still fires.
+            # TODO: Save and await this task.
+            asyncio.create_task(cb(chunk))  # type: ignore[unused-awaitable]


task can get gc'ed before complete

nevermind, missed comment above it

GC concern still open. Comment above explains scheduling, not GC-safety. Task reference dropped; may be collected mid-flight. Handler exceptions also dropped (unretrieved-task warning, no close()). TODO unresolved. Suggest holding a strong ref set with a done callback.

This is only for the trace, and it didn't get a chance to trace at all before, so it's probably fine to leave for the moment. I'm hoping we get a task scheduler thing like aiojobs included into asyncio in the future.

bdraco · 2026-06-13T02:44:04Z


+    async def _fire_chunk_received(self, chunk: bytes) -> None:
+        cb = self._on_chunk_received
+        assert cb is not None


assuming this is only for type narrowing

Yes. Type narrowing only. Callers guard self._on_chunk_received is not None before calling _fire_chunk_received. Assert never trips at runtime.

Minor optimisation by having the check outside the function, so the no-traces case doesn't need to create a coroutine.

aiolibsbot · 2026-06-13T02:51:56Z

PR Review — Add chunk tracing to .content

Solid refactor that moves on_response_chunk_received to the StreamReader level so direct .content reads are traced. Fire-once semantics across the read methods are carefully avoided (no double-fire), tests are thorough, and the response→payload cycle is cleared eagerly.

One real gap: read_nowait's fire-and-forget task can be GC'd, swallows handler exceptions, and isn't timer-bounded (acknowledged TODO).
Two static-analysis nits worth resolving: the raising setter on EmptyStreamReader (flagged by CodeQL + code-quality) and an unused Awaitable import.
Intentional behavior change: empty-body responses no longer fire the signal — documented in the changelog and docs, fine for 4.0.
bdraco's open questions (assert narrowing, CancelledError handling) addressed in replies.

🟡 Important

1. Fire-and-forget task in read_nowait can be GC'd and swallows exceptions (`aiohttp/streams.py`, L533-541)

read_nowait schedules the hook with a bare asyncio.create_task(cb(chunk)) whose reference is dropped immediately. Per the asyncio docs this task may be garbage-collected mid-flight, so the trace event can silently never run.

Three gaps versus the async paths:

GC risk: no strong reference is held. bdraco retracted his comment here, but the comment above only explains why the call is scheduled — it does not make the task GC-safe. The # TODO: Save and await this task. confirms this is still open.
Exception handling: a raising handler produces an unretrieved-task-exception warning rather than tearing down the response (the async paths propagate / close()).
No timer bound: unlike _fire_chunk_received, the scheduled task is not run under self._timer, so a hung handler is unbounded.

Minimal fix: keep a module/instance-level set of pending tasks, add the task, and add_done_callback(tasks.discard) so it survives until completion and exceptions surface.

chunk = self._read_nowait(n)
if chunk and (cb := self._on_chunk_received) is not None:
    # TODO: Save and await this task.
    asyncio.create_task(cb(chunk))  # type: ignore[unused-awaitable]

Checklist

No double-firing across read methods
Error paths tear down response / propagate — warning ＃1
No resource/task leaks — warning ＃1
Behavior change documented (changelog + docs)
Test coverage for new branches
No unused imports / static-analysis clean — suggestion ＃3
Singleton EMPTY_PAYLOAD isolation preserved

Silent Failure Analysis

🟠 **HIGH** — fire-and-forget async task (`aiohttp/streams.py:536-541`)

Risk: The scheduled task is never referenced, awaited, or given a done-callback, so any exception from the trace handler (including the self.close()/re-raise path in _on_chunk_response_received) is swallowed as an unhandled task exception, and the loop's weak reference means the task can even be garbage-collected before it runs — silently dropping the observability event entirely.

chunk = self._read_nowait(n)
if chunk and (cb := self._on_chunk_received) is not None:
    # TODO: Save and await this task.
    asyncio.create_task(cb(chunk))  # type: ignore[unused-awaitable]
return chunk

Fix: Store the task in a strong reference set (e.g. self._tasks), attach a done-callback that surfaces/logs exceptions and discards the reference, so failures are neither lost nor at risk of GC.

Automated review by Kōan (Claude) HEAD=9ff2508 3 min 56s

Add chunk tracing to .content

a78bcbd

Dreamsorcerer requested review from asvetlov and webknjaz as code owners June 10, 2026 01:14

Dreamsorcerer added the backport:skip Skip backport bot label Jun 10, 2026

psf-chronographer Bot added bot:chronographer:provided There is a change note present in this PR labels Jun 10, 2026

Rename 13000.bugfix.rst to 12899.bugfix.rst

97a11a2

github-code-quality Bot found potential problems Jun 10, 2026

View reviewed changes

Comment thread tests/test_streams.py Dismissed

Make EmptyPayload read-only

79dfaf5

github-code-quality Bot found potential problems Jun 10, 2026

View reviewed changes

Comment thread aiohttp/streams.py Fixed

github-advanced-security AI found potential problems Jun 10, 2026

View reviewed changes

Comment thread aiohttp/streams.py Fixed

Dreamsorcerer mentioned this pull request Jun 10, 2026

feat: ClientResponse class support methods iter_chunked and iter_chunks. #7455

Closed

5 tasks

Dreamsorcerer and others added 6 commits June 10, 2026 02:49

Fix type error

5b24db5

Rename 12899.bugfix.rst to 12889.bugfix.rst

cd32ba0

Improvements

79ca915

Fixes

dacff1b

[pre-commit.ci] auto fixes from pre-commit.com hooks

03b97c3

for more information, see https://pre-commit.ci

Update client_reqrep.py

e261e6f

github-advanced-security AI found potential problems Jun 11, 2026

View reviewed changes

Comment thread tests/test_streams.py Fixed

github-code-quality Bot found potential problems Jun 11, 2026

View reviewed changes

Comment thread tests/test_streams.py Dismissed

Comment thread tests/test_streams.py Fixed

Potential fix for pull request finding 'CodeQL / Unused import'

9ff2508

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

bdraco reviewed Jun 13, 2026

View reviewed changes

Uh oh!

Uh oh!

Conversation

Dreamsorcerer commented Jun 10, 2026

Uh oh!

codecov Bot commented Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codspeed-hq Bot commented Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Footnotes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dreamsorcerer Jun 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aiolibsbot commented Jun 13, 2026

PR Review — Add chunk tracing to .content

🟡 Important

Checklist

Silent Failure Analysis

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Jun 10, 2026 •

edited

Loading

codspeed-hq Bot commented Jun 10, 2026 •

edited

Loading

Dreamsorcerer Jun 13, 2026 •

edited

Loading