-
Notifications
You must be signed in to change notification settings - Fork 98
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
feat(server): Add support for async server function, module.server, and express module #1842
base: main
Are you sure you want to change the base?
Changes from all commits
3662f6c
04bf6b8
aaa9c6e
7162f5d
f1aa19c
88bdf4b
03721bb
58c0db5
eacb07b
d6b2420
cbb3242
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,17 @@ | |
from contextlib import AsyncExitStack, asynccontextmanager | ||
from inspect import signature | ||
from pathlib import Path | ||
from typing import Any, Callable, Literal, Mapping, Optional, TypeVar, cast | ||
from typing import ( | ||
Any, | ||
Awaitable, | ||
Callable, | ||
Literal, | ||
Mapping, | ||
Optional, | ||
TypeVar, | ||
Union, | ||
cast, | ||
) | ||
|
||
import starlette.applications | ||
import starlette.exceptions | ||
|
@@ -29,7 +39,7 @@ | |
from ._connection import Connection, StarletteConnection | ||
from ._error import ErrorMiddleware | ||
from ._shinyenv import is_pyodide | ||
from ._utils import guess_mime_type, is_async_callable, sort_keys_length | ||
from ._utils import guess_mime_type, is_async_callable, sort_keys_length, wrap_async | ||
from .bookmark import _global as bookmark_global_state | ||
from .bookmark._global import as_bookmark_dir_fn | ||
from .bookmark._restore_state import RestoreContext, restore_context | ||
|
@@ -66,8 +76,8 @@ class App: | |
returns a UI definition, if you need the UI definition to be created dynamically | ||
for each pageview. | ||
server | ||
A function which is called once for each session, ensuring that each session is | ||
independent. | ||
A sync or async function which is called once for each session, ensuring that | ||
each session is independent. | ||
static_assets | ||
Static files to be served by the app. If this is a string or Path object, it | ||
must be a directory, and it will be mounted at `/`. If this is a dictionary, | ||
|
@@ -113,7 +123,7 @@ def server(input: Inputs, output: Outputs, session: Session): | |
""" | ||
|
||
ui: RenderedHTML | Callable[[Request], Tag | TagList] | ||
server: Callable[[Inputs, Outputs, Session], None] | ||
server: Callable[[Inputs, Outputs, Session], Awaitable[None]] | ||
|
||
_bookmark_save_dir_fn: BookmarkSaveDirFn | None | ||
_bookmark_restore_dir_fn: BookmarkRestoreDirFn | None | ||
|
@@ -123,7 +133,9 @@ def __init__( | |
self, | ||
ui: Tag | TagList | Callable[[Request], Tag | TagList] | Path, | ||
server: ( | ||
Callable[[Inputs], None] | Callable[[Inputs, Outputs, Session], None] | None | ||
Callable[[Inputs], Awaitable[None] | None] | ||
| Callable[[Inputs, Outputs, Session], Awaitable[None] | None] | ||
| None | ||
Comment on lines
+136
to
+138
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because this can be defined with an async function, can we use This will always upgrade the function to async. Then in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current By wrapping all server functions to be async, we won't need to check if we need to await or not (as we'll always need to await). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sweet. Makes sense to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Though, now looking into support server function to be async, would need to go down the rabbit hole of ensure that we tackle other side effects like the module.server() decorator which is not async. Could leave it only supporting sync i suppose.. curious on thoughts from @wch. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Happy to have all server-like functions upgraded to async There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @schloerke - it makes sense to make everything async-capable. Hopefully that's not too much more work. |
||
), | ||
*, | ||
static_assets: Optional[str | Path | Mapping[str, str | Path]] = None, | ||
|
@@ -136,13 +148,20 @@ def __init__( | |
self._exit_stack = AsyncExitStack() | ||
|
||
if server is None: | ||
self.server = noop_server_fn | ||
self.server = wrap_async(noop_server_fn) | ||
elif len(signature(server).parameters) == 1: | ||
self.server = wrap_server_fn_with_output_session( | ||
cast(Callable[[Inputs], None], server) | ||
wrap_async( | ||
cast(Callable[[Inputs], Union[Awaitable[None], None]], server) | ||
) | ||
) | ||
elif len(signature(server).parameters) == 3: | ||
self.server = cast(Callable[[Inputs, Outputs, Session], None], server) | ||
self.server = wrap_async( | ||
cast( | ||
Callable[[Inputs, Outputs, Session], Union[Awaitable[None], None]], | ||
server, | ||
) | ||
) | ||
else: | ||
raise ValueError( | ||
"`server` must have 1 (Inputs) or 3 parameters (Inputs, Outputs, Session)" | ||
|
@@ -571,10 +590,10 @@ def noop_server_fn(input: Inputs, output: Outputs, session: Session) -> None: | |
|
||
|
||
def wrap_server_fn_with_output_session( | ||
server: Callable[[Inputs], None], | ||
) -> Callable[[Inputs, Outputs, Session], None]: | ||
def _server(input: Inputs, output: Outputs, session: Session): | ||
server: Callable[[Inputs], Awaitable[None]], | ||
) -> Callable[[Inputs, Outputs, Session], Awaitable[None]]: | ||
async def _server(input: Inputs, output: Outputs, session: Session): | ||
# Only has 1 parameter, ignore output, session | ||
server(input) | ||
await server(input) | ||
|
||
return _server |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -262,15 +262,15 @@ def private_seed() -> Generator[None, None, None]: | |
|
||
|
||
def wrap_async( | ||
fn: Callable[P, R] | Callable[P, Awaitable[R]], | ||
fn: Callable[P, R] | Callable[P, Awaitable[R]] | Callable[P, Awaitable[R] | R], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the change on this line shouldn't be necessary, after making the changes above in the signature of I could be wrong, but I don't believe it's possible for a function in Python to have a return value of |
||
) -> Callable[P, Awaitable[R]]: | ||
""" | ||
Given a synchronous function that returns R, return an async function that wraps the | ||
original function. If the input function is already async, then return it unchanged. | ||
""" | ||
|
||
if is_async_callable(fn): | ||
return fn | ||
return cast(Callable[P, Awaitable[R]], fn) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this change necessary? |
||
|
||
fn = cast(Callable[P, R], fn) | ||
|
||
|
@@ -362,10 +362,10 @@ def is_async_callable( | |
return False | ||
|
||
|
||
# def not_is_async_callable( | ||
# obj: Callable[P, T] | Callable[P, Awaitable[T]] | ||
# ) -> TypeGuard[Callable[P, T]]: | ||
# return not is_async_callable(obj) | ||
def not_is_async_callable( | ||
obj: Callable[P, T] | Callable[P, Awaitable[T]], | ||
) -> TypeGuard[Callable[P, T]]: | ||
return not is_async_callable(obj) | ||
Comment on lines
+365
to
+368
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of adding this function, can we change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just saw the comment below about how |
||
|
||
|
||
# See https://stackoverflow.com/a/59780868/412655 for an excellent explanation | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cosmetic split.
I remember something from past return types that it's a little better to split
Awaitable
from the return type as it can't actually return both, but it is really two independent types.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, they are different types and need separate entries for correctness.