From 22774aefc9c26163c148bc9909750742a64e1c49 Mon Sep 17 00:00:00 2001 From: Joe Cheng Date: Tue, 12 Nov 2024 15:36:33 -0800 Subject: [PATCH 1/6] Initial sketch of {websockets} 14.0 support Some types changed, and process_request is totally different. However, this still leaves a spurious, scary-looking error when using the Run App button from the current version of the Shiny VS Code extension. --- shiny/_autoreload.py | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/shiny/_autoreload.py b/shiny/_autoreload.py index b77fec6fe..66a2e9589 100644 --- a/shiny/_autoreload.py +++ b/shiny/_autoreload.py @@ -70,7 +70,7 @@ def reload_end(): async def _() -> None: options = { - "extra_headers": { + "additional_headers": { "Shiny-Autoreload-Secret": os.getenv("SHINY_AUTORELOAD_SECRET", ""), } } @@ -186,6 +186,8 @@ async def _coro_main( port: int, app_url: str, secret: str, launch_browser: bool ) -> None: import websockets + import websockets.asyncio.server + import websockets.http11 reload_now: asyncio.Event = asyncio.Event() @@ -198,18 +200,22 @@ def nudge(): reload_now.set() reload_now.clear() - async def reload_server(conn: websockets.server.WebSocketServerProtocol): + async def reload_server(conn: websockets.asyncio.server.ServerConnection): try: - if conn.path == "/autoreload": + if conn.request is None: + raise RuntimeError( + "Autoreload server received a connection with no request" + ) + elif conn.request.path == "/autoreload": # The client wants to be notified when the app has reloaded. The client # in this case is the web browser, specifically shiny-autoreload.js. while True: await reload_now.wait() await conn.send("autoreload") - elif conn.path == "/notify": + elif conn.request.path == "/notify": # The client is notifying us that the app has reloaded. The client in # this case is the uvicorn worker process (see reload_end(), above). - req_secret = conn.request_headers.get("Shiny-Autoreload-Secret", "") + req_secret = conn.request.headers.get("Shiny-Autoreload-Secret", "") if req_secret != secret: # The client couldn't prove that they were from a child process return @@ -224,7 +230,7 @@ async def reload_server(conn: websockets.server.WebSocketServerProtocol): # about only WebSockets being supported. This is not an academic problem as the # VSCode extension used in RSW sniffs out ports that are being listened on, which # leads to confusion if all you get is an error. - async def process_request( + async def process_request_legacy( path: str, request_headers: websockets.datastructures.Headers ) -> Optional[tuple[http.HTTPStatus, websockets.datastructures.HeadersLike, bytes]]: # If there's no Upgrade header, it's not a WebSocket request. @@ -236,8 +242,22 @@ async def process_request( await asyncio.sleep(1) return (http.HTTPStatus.MOVED_PERMANENTLY, [("Location", app_url)], b"") + async def process_request_new( + connection: websockets.asyncio.server.ServerConnection, + request: websockets.http11.Request, + ) -> websockets.http11.Response | None: + if request.headers.get("Upgrade") is None: + return websockets.http11.Response( + status_code=http.HTTPStatus.MOVED_PERMANENTLY, + reason_phrase="Moved Permanently", + headers=websockets.Headers(Location=app_url), + body=None, + ) + else: + return None + async with websockets.serve( - reload_server, "127.0.0.1", port, process_request=process_request + reload_server, "127.0.0.1", port, process_request=process_request_new ): await asyncio.Future() # wait forever From ad9995d30b79d4dc7fef52ee73e4f4da56458138 Mon Sep 17 00:00:00 2001 From: Joe Cheng Date: Tue, 12 Nov 2024 16:22:35 -0800 Subject: [PATCH 2/6] Squelch spurious autoreload errors --- CHANGELOG.md | 2 ++ shiny/_autoreload.py | 31 ++++++++++++++++++++++++++----- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7eca908a3..8fcfbaef8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * Branded theming via `ui.Theme.from_brand()` now correctly applies monospace inline and block font family choices. (#1762) +* Compatibility with `websockets>=14.0`, which has changed its public APIs (#1769). + ## [1.2.0] - 2024-10-29 diff --git a/shiny/_autoreload.py b/shiny/_autoreload.py index 66a2e9589..1d0693b7e 100644 --- a/shiny/_autoreload.py +++ b/shiny/_autoreload.py @@ -169,21 +169,36 @@ def start_server(port: int, app_port: int, launch_browser: bool): os.environ["SHINY_AUTORELOAD_PORT"] = str(port) os.environ["SHINY_AUTORELOAD_SECRET"] = secret + # websockets 14.0 (and presumably later) log an error if a connection is opened and + # closed before any data is sent. Our VS Code extension does exactly this--opens a + # connection to check if the server is running, then closes it. It's better that it + # does this and doesn't actually perform an HTTP request because we can't guarantee + # that the HTTP request will be cheap (we do the same ping on both the autoreload + # socket and the main uvicorn socket). So better to just suppress all errors until + # we think we have a problem. You can unsuppress by setting the environment variable + # to DEBUG. + loglevel = os.getenv("SHINY_AUTORELOAD_LOG_LEVEL", "CRITICAL") + app_url = get_proxy_url(f"http://127.0.0.1:{app_port}/") # Run on a background thread so our event loop doesn't interfere with uvicorn. # Set daemon=True because we don't want to keep the process alive with this thread. threading.Thread( - None, _thread_main, args=[port, app_url, secret, launch_browser], daemon=True + None, + _thread_main, + args=[port, app_url, secret, launch_browser, loglevel], + daemon=True, ).start() -def _thread_main(port: int, app_url: str, secret: str, launch_browser: bool): - asyncio.run(_coro_main(port, app_url, secret, launch_browser)) +def _thread_main( + port: int, app_url: str, secret: str, launch_browser: bool, loglevel: str +): + asyncio.run(_coro_main(port, app_url, secret, launch_browser, loglevel)) async def _coro_main( - port: int, app_url: str, secret: str, launch_browser: bool + port: int, app_url: str, secret: str, launch_browser: bool, loglevel: str ) -> None: import websockets import websockets.asyncio.server @@ -256,8 +271,14 @@ async def process_request_new( else: return None + # logging.getLogger("websockets").addHandler(logging.NullHandler()) + logging.getLogger("websockets").setLevel(loglevel) + async with websockets.serve( - reload_server, "127.0.0.1", port, process_request=process_request_new + reload_server, + "127.0.0.1", + port, + process_request=process_request_new, ): await asyncio.Future() # wait forever From 5d6c4493199281c2d84db32139cbfde85dc0f650 Mon Sep 17 00:00:00 2001 From: Joe Cheng Date: Tue, 12 Nov 2024 16:51:30 -0800 Subject: [PATCH 3/6] Tweak autoreload logic to be compatible with websockets>=13.0 --- pyproject.toml | 2 +- shiny/_autoreload.py | 32 +++++++++----------------------- 2 files changed, 10 insertions(+), 24 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index e9778149b..291849ea2 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -31,7 +31,7 @@ dependencies = [ "typing-extensions>=4.10.0", "uvicorn>=0.16.0;platform_system!='Emscripten'", "starlette", - "websockets>=10.0", + "websockets>=13.0", "python-multipart", "htmltools>=0.6.0", "click>=8.1.4;platform_system!='Emscripten'", diff --git a/shiny/_autoreload.py b/shiny/_autoreload.py index 1d0693b7e..361f5a1bb 100644 --- a/shiny/_autoreload.py +++ b/shiny/_autoreload.py @@ -59,6 +59,7 @@ def reload_begin(): # Called from child process when new application instance starts up def reload_end(): import websockets + import websockets.asyncio.client # os.kill(os.getppid(), signal.SIGUSR1) @@ -75,7 +76,7 @@ async def _() -> None: } } try: - async with websockets.connect( + async with websockets.asyncio.client.connect( url, **options # pyright: ignore[reportArgumentType] ) as websocket: await websocket.send("reload_end") @@ -202,7 +203,7 @@ async def _coro_main( ) -> None: import websockets import websockets.asyncio.server - import websockets.http11 + from websockets.http11 import Request, Response reload_now: asyncio.Event = asyncio.Event() @@ -245,24 +246,12 @@ async def reload_server(conn: websockets.asyncio.server.ServerConnection): # about only WebSockets being supported. This is not an academic problem as the # VSCode extension used in RSW sniffs out ports that are being listened on, which # leads to confusion if all you get is an error. - async def process_request_legacy( - path: str, request_headers: websockets.datastructures.Headers - ) -> Optional[tuple[http.HTTPStatus, websockets.datastructures.HeadersLike, bytes]]: - # If there's no Upgrade header, it's not a WebSocket request. - if request_headers.get("Upgrade") is None: - # For some unknown reason, this fixes a tendency on GitHub Codespaces to - # correctly proxy through this request, but give a 404 when the redirect is - # followed and app_url is requested. With the sleep, both requests tend to - # succeed reliably. - await asyncio.sleep(1) - return (http.HTTPStatus.MOVED_PERMANENTLY, [("Location", app_url)], b"") - - async def process_request_new( + async def process_request( connection: websockets.asyncio.server.ServerConnection, - request: websockets.http11.Request, - ) -> websockets.http11.Response | None: + request: Request, + ) -> Response | None: if request.headers.get("Upgrade") is None: - return websockets.http11.Response( + return Response( status_code=http.HTTPStatus.MOVED_PERMANENTLY, reason_phrase="Moved Permanently", headers=websockets.Headers(Location=app_url), @@ -274,11 +263,8 @@ async def process_request_new( # logging.getLogger("websockets").addHandler(logging.NullHandler()) logging.getLogger("websockets").setLevel(loglevel) - async with websockets.serve( - reload_server, - "127.0.0.1", - port, - process_request=process_request_new, + async with websockets.asyncio.server.serve( + reload_server, "127.0.0.1", port, process_request=process_request ): await asyncio.Future() # wait forever From 4e186ac1da3140e490b52accfce3a9e6d1b50111 Mon Sep 17 00:00:00 2001 From: Joe Cheng Date: Tue, 12 Nov 2024 17:44:02 -0800 Subject: [PATCH 4/6] Simplify loglevel setting No need to pass through to the background thread, we can just change the loglevel on the main thread. --- shiny/_autoreload.py | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/shiny/_autoreload.py b/shiny/_autoreload.py index 361f5a1bb..f50946f10 100644 --- a/shiny/_autoreload.py +++ b/shiny/_autoreload.py @@ -179,6 +179,7 @@ def start_server(port: int, app_port: int, launch_browser: bool): # we think we have a problem. You can unsuppress by setting the environment variable # to DEBUG. loglevel = os.getenv("SHINY_AUTORELOAD_LOG_LEVEL", "CRITICAL") + logging.getLogger("websockets").setLevel(loglevel) app_url = get_proxy_url(f"http://127.0.0.1:{app_port}/") @@ -187,19 +188,17 @@ def start_server(port: int, app_port: int, launch_browser: bool): threading.Thread( None, _thread_main, - args=[port, app_url, secret, launch_browser, loglevel], + args=[port, app_url, secret, launch_browser], daemon=True, ).start() -def _thread_main( - port: int, app_url: str, secret: str, launch_browser: bool, loglevel: str -): - asyncio.run(_coro_main(port, app_url, secret, launch_browser, loglevel)) +def _thread_main(port: int, app_url: str, secret: str, launch_browser: bool): + asyncio.run(_coro_main(port, app_url, secret, launch_browser)) async def _coro_main( - port: int, app_url: str, secret: str, launch_browser: bool, loglevel: str + port: int, app_url: str, secret: str, launch_browser: bool ) -> None: import websockets import websockets.asyncio.server @@ -260,9 +259,6 @@ async def process_request( else: return None - # logging.getLogger("websockets").addHandler(logging.NullHandler()) - logging.getLogger("websockets").setLevel(loglevel) - async with websockets.asyncio.server.serve( reload_server, "127.0.0.1", port, process_request=process_request ): From 3ac6ef023f0d656636ff1d9571ff8ca3912ca289 Mon Sep 17 00:00:00 2001 From: Joe Cheng Date: Tue, 12 Nov 2024 17:45:55 -0800 Subject: [PATCH 5/6] Slight code formatting, to minimize diff --- shiny/_autoreload.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/shiny/_autoreload.py b/shiny/_autoreload.py index f50946f10..634554608 100644 --- a/shiny/_autoreload.py +++ b/shiny/_autoreload.py @@ -186,10 +186,7 @@ def start_server(port: int, app_port: int, launch_browser: bool): # Run on a background thread so our event loop doesn't interfere with uvicorn. # Set daemon=True because we don't want to keep the process alive with this thread. threading.Thread( - None, - _thread_main, - args=[port, app_url, secret, launch_browser], - daemon=True, + None, _thread_main, args=[port, app_url, secret, launch_browser], daemon=True ).start() From 671616d775315c1bb65f2783f38000020dd63c1a Mon Sep 17 00:00:00 2001 From: Garrick Aden-Buie Date: Wed, 13 Nov 2024 13:00:16 -0500 Subject: [PATCH 6/6] docs: Update changelog item --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8fcfbaef8..d26427fd9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * Branded theming via `ui.Theme.from_brand()` now correctly applies monospace inline and block font family choices. (#1762) -* Compatibility with `websockets>=14.0`, which has changed its public APIs (#1769). +* Compatibility with `websockets>=14.0`, which has changed its public APIs. Shiny now requires websockets 13 or later (#1769). ## [1.2.0] - 2024-10-29