Skip to content

Close Request.stream() generators with aclosing - #3628

Merged
Kludex merged 1 commit into
Kludex:mainfrom
graingert:aclose-request-stream
Oct 5, 2026
Merged

Kludex merged 1 commit into
Kludex:mainfrom
graingert:aclose-request-stream

Conversation

@graingert

@graingert graingert commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Summary

_CachedRequest.wrapped_receive (used by BaseHTTPMiddleware) creates a new Request.stream() async generator on every call, takes one chunk with __anext__(), and drops it while it is still suspended. Each one is then finalized by the GC through the event loop's asyncgen finalizer hook. Request.body() and the form parsers in Request._get_form() can leak the same way when parsing raises (e.g. MultiPartException for too many files) or is cancelled mid-stream.

This is the leak behind:

It also provides the reproducer that #3093 was missing for the unused _wrapped_rc_stream.

Changes:

  • Wrap every internal use of self.stream() (wrapped_receive, body(), both form parsers) in contextlib.aclosing. Closing a stream generator after one chunk is safe: its position lives on the request (_receive / _stream_consumed), not in the generator, and nothing runs on GeneratorExit.
  • Drop the unused _wrapped_rc_stream.
  • Remove the filterwarnings ignore for trio's Async generator 'starlette.requests.Request.stream' was garbage collected... ResourceWarning. With the ignore removed, 21 tests fail on main; with this change the full suite passes, so the trio test runs now guard against regressions.
  • Fix tests that leaked their own async generators (unclosed receive() generators, and breaking out of request.stream()), which the stricter warning config surfaced.

User code that breaks out of async for chunk in request.stream() can still leak a generator; it may be worth a docs note recommending aclosing there as a follow-up.

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

🤖 Generated with Claude Code

Review in cubic

`_CachedRequest.wrapped_receive` created a new `Request.stream()`
generator on every call, took one chunk and abandoned it suspended,
leaving it to be finalized by the GC. `body()` and the form parsers
could do the same on error or cancellation. Under uvloop debug mode
the asyncgen finalizer hook can segfault when run from GC
(MagicStack/uvloop#756), and earlier surfaced as the traceback in Kludex#3066.

Wrap every internal use of `self.stream()` in `contextlib.aclosing`,
drop the unused `_wrapped_rc_stream`, and remove the pytest filter that
was suppressing trio's ResourceWarning for this leak so it can't regress.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 4 files

Re-trigger cubic

@codspeed

codspeed Bot commented Oct 5, 2026

Copy link
Copy Markdown

Merging this PR will improve performance by 63.8%

⚡ 6 improved benchmarks
✅ 236 untouched benchmarks

Performance Changes

Mode Benchmark BASE HEAD Efficiency
⚡ Memory test_multipart[max-fields] 4,295 B 864 B ×5
⚡ Memory test_multipart[max-part-size] 4.2 KB 2 KB ×2.1
⚡ Simulation test_multipart[file-512KiB] 3.6 ms 2.8 ms +28.41%
⚡ Simulation test_urlencoded_form[max-part-size] 558 µs 468.5 µs +19.1%
⚡ Simulation test_multipart[boundary-like-file] 6.8 ms 6.1 ms +11.68%
⚡ Simulation test_multipart[max-part-size] 848.9 µs 770.2 µs +10.22%

Tip

Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.


Comparing graingert:aclose-request-stream (1a7a23f) with main (7cb2f09)

Open in CodSpeed

@Kludex
Kludex merged commit ff5b4df into Kludex:main Oct 5, 2026
12 checks passed
@Kludex

Kludex commented Oct 5, 2026

Copy link
Copy Markdown
Owner

Thanks!

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.

2 participants