Fix Python ASGI adaptor to handle streaming responses correctly

[ryanking13]

Resolves cloudflare/workers-py#67

Our ASGI adaptor is not handling streaming responses correctly for non-SSE cases. We can basically apply the same thing in SSE and non-SSE (waiting until the stream is all consumed) by calling ctx.waitUntil. Therefore this PR removes the is_sse branch and use ctx.waitUntil for both SSE and non-SSE cases.

However, that can be a breaking change, as ctx parameter is optional (at least for now), so I added a separate branch to pre-consume all the response inside ASGI when ctx is not available.

[ask-bonk]

I'm Bonk, and I've done a quick review of your PR.

This PR fixes the ASGI adaptor so that non-SSE streaming responses (with more_body=True) are no longer silently truncated to their first chunk, by using ctx.waitUntil plus a TransformStream when ctx is available, and falling back to full buffering when it is not.

Issues

1. Dead code — SSE-without-ctx error is unreachable (medium): On line 238, if is_sse and ctx is None is inside the if use_streaming: branch, but use_streaming is ctx is not None (line 122). So ctx is None is always False here, making this guard dead code. The backwards-compatible RuntimeError for SSE-without-ctx that the PR description mentions preserving will never fire — instead the caller silently gets a buffered SSE response. If the intent is to keep that error, the check needs to move before/outside the use_streaming branch.

2. Typo (low): Line 198 has "inefficident" — should be "inefficient".

[ask-bonk]

use_streaming is ctx is not None (line 122), so inside this if use_streaming: branch ctx is guaranteed non-None. The if is_sse and ctx is None guard is dead code and the RuntimeError for SSE-without-ctx will never fire.

If you want to preserve the error for callers who omit ctx and happen to get an SSE response, you could move the check before the branch:

Suggested change

	if use_streaming:
	# Let the application continue running in the background to stream
	# the response body via the TransformStream.
	if is_sse and ctx is None:
	raise RuntimeError(
	"Server-Side-Events require ctx to be passed to asgi.fetch"
	)
	if is_sse and ctx is None:
	raise RuntimeError(
	"Server-Side-Events require ctx to be passed to asgi.fetch"
	)
	if use_streaming:
	# Let the application continue running in the background to stream
	# the response body via the TransformStream.
	ctx.waitUntil(create_proxy(app_task))

[ask-bonk]

Tiny typo:

Suggested change

	# the runtime handle the streaming.
	# TODO(soon): This is inefficient, make `ctx` mandatory and let

[ask-bonk]

I'm Bonk, and I've done a quick review of your PR. I posted a review with 2 findings:

1. Dead code (medium): The if is_sse and ctx is None check on line 238 is inside the if use_streaming: branch, but use_streaming = ctx is not None, so ctx is None is always False there. The backwards-compatible RuntimeError for SSE-without-ctx will never fire. I posted a suggestion to move the check before the use_streaming branch.

2. Typo (low): "inefficident" → "inefficient" on line 198, with a suggestion fix.

github run

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.63%. Comparing base (acdca96) to head (c7d3d02).
⚠️ Report is 151 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6173      +/-   ##
==========================================
- Coverage   70.70%   70.63%   -0.07%     
==========================================
  Files         409      413       +4     
  Lines      109411   109821     +410     
  Branches    18018    18097      +79     
==========================================
+ Hits        77356    77570     +214     
- Misses      21246    21438     +192     
- Partials    10809    10813       +4     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[dom96]

What's the use case for supporting this when ctx isn't specified? It's a user error to not pass it in, isn't it? Why don't we instead throw an error if it isn't specified?

[dom96]

Ahh, I see you've got a TODO for this

[ryanking13]

It's a user error to not pass it in, isn't it?

Well, I found that we don't document passing ctx anywhere. (1, 2 ). So it is our fault 😂.

I think we should make that parameter as required, but there should be workers in production lying around so we cannot make that change without compat flag.

Actually, after we unvendor the Python SDK from the workerd, it can be done by releasing a new SDK version, but that's not done yet.

[danlapid]

We shipped importable waitUntil for javascript a while back, we should ship it for python as well, similar to import env from workers and then remove the dependency on this being passed in as a parameter

[ryanking13]

I see. That sounds more sense. Let me see if I can do that instead of raising error when ctx is not passed.

[danlapid]

Well, now I feel shitty for only doing this for the SSE branch