align datastar_response behavior with docs (tests, fixes, examples)#27
Conversation
c1e0d12 to
2d48f42
Compare
|
Thanks for this. Just want to verify what it's fixing first though, what were the cases that weren't working as expected? |
2d48f42 to
2235325
Compare
|
@gazpachoking (Thanks for maintaining this and) thanks for the question; I should have been clearer, and this prodded me to get more specific about the issue. I had initially said the decorator ‘didn’t work as expected’, which was too broad; the narrower issue is it returned a coroutine for sync handlers, so blocking work ran on the event loop. I use fasthtml, and I think I experienced that as the decorator "not working", when it was more likely things bogging down for me under load because of the sync/async issue. In this updated PR, I add some "matrix" tests, that establish that the decorator works/has worked for {sync, async} x {generator, not-generator} x {frameworks}. But I also add a test that shows the async/sync issue. Matrix tests pass under 0.7.0; the concurrency test fails under 0.7.0 and passes with this fix. The third commit makes minor updates to the docs/egs. I'm no expert in async/sync stuff, just a fasthtml + datastar user. |
|
Hmm. I see, I hadn't considered that but I guess transforming a sync handler into async could be surprising. Don't we have the opposite problem with this fix though, even async handlers will be turned into sync ones, thus causing a thread to be spawned? |
|
@mmacpherson What about something like this? @overload
def datastar_response(
func: Callable[P, Coroutine[DatastarEvent | None, None, DatastarEvents | None]],
) -> Callable[P, Coroutine[None, None, DatastarResponse]]: ...
@overload
def datastar_response(func: Callable[P, DatastarEvents]) -> Callable[P, DatastarResponse]: ...
def datastar_response(
func: Callable[P, Coroutine[DatastarEvent | None, None, DatastarEvents] | DatastarEvents],
) -> Callable[P, Coroutine[None, None, DatastarResponse] | DatastarResponse]:
"""A decorator which wraps a function result in DatastarResponse.
Can be used on a sync or async function or generator function.
"""
if inspect.iscoroutinefunction(func):
@wraps(func)
async def wrapper(*args: P.args, **kwargs: P.kwargs) -> DatastarResponse:
return DatastarResponse(await func(*args, **kwargs))
else:
@wraps(func)
def wrapper(*args: P.args, **kwargs: P.kwargs) -> DatastarResponse:
return DatastarResponse(func(*args, **kwargs))
wrapper.__annotations__["return"] = DatastarResponse
return wrapperThis method would keep the function sync or async as the user defined it. Want to try this out? |
75b6079 to
598e862
Compare
|
@gazpachoking Thanks for this—you're right that wrapping async handlers in sync wrappers causes unnecessary threadpool overhead. However, I noticed a gap in the proposed fix: I’ve refined the approach to unwind # Unwrap partials to inspect the actual underlying function
actual_func = func
while isinstance(actual_func, partial):
actual_func = actual_func.func
# Case A: Async Generator (async def + yield)
if isasyncgenfunction(actual_func):
@wraps(actual_func)
async def async_gen_wrapper(*args: P.args, **kwargs: P.kwargs) -> DatastarResponse:
return DatastarResponse(func(*args, **kwargs))
async_gen_wrapper.__annotations__["return"] = DatastarResponse
return async_gen_wrapper
# Case B: Standard Coroutine (async def + return)
elif iscoroutinefunction(actual_func):
@wraps(actual_func)
async def async_coro_wrapper(*args: P.args, **kwargs: P.kwargs) -> DatastarResponse:
result = await func(*args, **kwargs)
return DatastarResponse(result)
async_coro_wrapper.__annotations__["return"] = DatastarResponse
return async_coro_wrapper
# Case C: Sync Function (def)
else:
@wraps(actual_func)
def sync_wrapper(*args: P.args, **kwargs: P.kwargs) -> DatastarResponse:
return DatastarResponse(func(*args, **kwargs))
sync_wrapper.__annotations__["return"] = DatastarResponse
return sync_wrapperI've verified that all the tests proposed in this PR pass with this change. Thoughts? |
|
I haven't forgotten this, and think it's worth fixing. I just want to get some time to test things out myself as well.
Hmm, both iscoroutinefunction and isasyncgenfunction both call out explicitly in the docs that they detect functools.partial methods. But more importantly, what's the situation that someone would be applying a decorator to a partial function? Just trying to simplify the implementation. Also, can't both the coroutine and async generator case follow the same code branch? (The same behavior we have now.) Just the sync case needs to be split out I think. |
|
Thanks for the patience and the feedback! You're right on both counts.
This reduces most implementations to two paths: Sync (threadpool) and Note: Quart needs to keep the async generator branch separate because of |
gazpachoking
left a comment
There was a problem hiding this comment.
Okay, finally had a chance to look closer at this. It seems like a good thing overall. A couple small comments in there.
The testing code seems like a lot, and seems like there is overlap between the different files. Can we maybe pare that down a bit? Tests are good, but it's a lot of code to grok and maintain. I'd be more inclined to skip the integration tests and just make sure the wrapped functions return the right thing.
| @wraps(func) | ||
| async def async_gen_wrapper(*args: P.args, **kwargs: P.kwargs) -> DatastarResponse: | ||
| return DatastarResponse(stream_with_context(func)(*args, **kwargs)) | ||
| return DatastarResponse(await copy_current_request_context(func)(*args, **kwargs)) |
There was a problem hiding this comment.
Why don't we need the copy_current_request_context anymore?
There was a problem hiding this comment.
Restored in ca51009. copy_current_request_context binds the handler to a copy of the current request context so quart.request / g stay accessible across the internal await. Left the sync branch alone for minimal diff.
| Preserves the sync/async nature of the decorated function. | ||
| """ | ||
| if isasyncgenfunction(func): | ||
| raise NotImplementedError( |
There was a problem hiding this comment.
What's the issue with async for django? It looks like it was working with an async wrapper before, why disable that?
There was a problem hiding this comment.
Fixed in ca51009. The original decorator passed async gens through without special-casing, and Django's StreamingHttpResponse supports async iteration under ASGI. Unified coroutine + async-gen into one async wrapper (matching starlette/litestar) and unskipped the Django async_generator case in the matrix test.
- remove unnecessary partial function check - exchange deco-time async generator vs coroutine check for runtime - make analogous changes for each supported framework - update test docstring to reflect implementation
90d9d41 to
add7b8d
Compare
…rs, trim tests - quart: restore copy_current_request_context in the coroutine branch (dropped in 90d9d41 without justification; reviewer flagged regression risk) - django: drop NotImplementedError for async generators; unify coroutine + async-gen into a single async wrapper. Django's StreamingHttpResponse supports async iteration under ASGI, so this restores prior permissive behavior - matrix test: unskip the Django async_generator case (now passes) - tests: remove per-framework live-server integration tests (fastapi, fasthtml, litestar, starlette); keep test_decorator_matrix.py for cross-framework contract coverage and test_sync_handler_runs_off_event_loop as the regression guard for the original bug Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Thanks @gazpachoking. Pushed ca51009 addressing all three points. Tests: dropped the 4 per-framework live-server integration files. test_decorator_matrix.py already covers each framework × handler-shape at the return-shape level, which was the main thing I'd been hoping to contribute — the package advertises multi-framework support and I didn't find coverage validating those claims. Kept test_sync_handler_runs_off_event_loop as the regression guard for the original bug (can't demonstrate it without a live server). Net: ~774 → ~190 lines. Quart: restored copy_current_request_context in the coroutine branch. Django: dropped the NotImplementedError. Rechecking the original decorator, async gens fell through to DatastarResponse(r) and Django's StreamingHttpResponse supports async iteration under ASGI, so they worked. Unified the coroutine + async-gen paths into one async wrapper matching starlette/litestar, and unskipped the Django async_generator case in the matrix test. Note: the Quart restore isn't exercised by any test — matrix skips Quart (needs a full request context) and there's no Quart integration test. Matches pre-PR behavior either way. |
|
Looks good! Do you mind fixing up and/or disabling the linting checks as needed? |
- add tests/** to ruff per-file-ignores for ANN, PLC0415, PLR2004 (mirrors existing examples/ treatment; these rules are noisy for test fixtures, conditional framework imports, and threshold values) - remove leftover unused Union import in sanic.py (rebase artifact) - apply ruff --fix and ruff format to test files - add `lint` target to Makefile Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5f6e761 to
2580f02
Compare
|
Thanks kindly @gazpachoking . Linting now passes for me locally, modulo a couple ignores I added to the tests/ directory, trying to match what was already there. I also added |
|
Thanks! |
|
Thanks kindly @gazpachoking , cheers. |
(PR statement heavily revised after initial submission; see comment below.)
Problem
@datastar_response returned a coroutine for sync handlers, so frameworks treated them as async and ran blocking work on the event loop. That stalls unrelated requests under load and produces “coroutine was never awaited” warnings if you call the decorated function directly.
Fix
Keep the wrapper sync and normalize the result: async iterable → DatastarResponse, sync iterable → DatastarResponse, awaitable → tiny async gen that awaits once → DatastarResponse, else wrap the value. Applied consistently across starlette/fastapi/fasthtml/litestar/django/sanic.
Sync handlers now stay in the threadpool; return shape is always DatastarResponse.
(sanic streams generators via request.respond; returns None there)
Tests
Docs
README notes the decorator preserves sync semantics.
Verification
All tests are green, warnings are upstream deprecations (litestar/websockets/py3.14).