-
Notifications
You must be signed in to change notification settings - Fork 7
align datastar_response behavior with docs (tests, fixes, examples) #27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
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? |
(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).