From 37bce20e31ecb95163877c9f4a1e8857f90a711c Mon Sep 17 00:00:00 2001 From: Max Schmitt Date: Mon, 26 Feb 2024 19:24:50 +0100 Subject: [PATCH] fix: report route handler errors to the user after they completed (#2321) Issue: Exceptions which will get thrown in route handlers after route.{fulfill,continue,abort} got called, won't get surfaced to the user in the sync and async version of Playwright for Python. Scope: Event handlers which use asyncio.create_task - {BrowserContext,Page}.on("route") - {Page}.on({"route", "locatorHandlerTriggered"}) There were multiple issues in the implementation: 1. `playwright/_impl/_helper.py` (sync only): we were only waiting until a handler's {continue,fulfill,abort} got called, and did not wait until the actual callback completed. Fix: Wait until the handler completed. 2. `playwright/_impl/_connection.py:423` (sync only): We call event listeners manually, this means that `pyee`'s `"error"` event is not working there. Fix: attach a done callback manually, like pyee is doing inside their library. 3. `playwright/_impl/_connection.py:56` (async): attach an `"error"` event handler for the async error reporting 4. `playwright/_impl/_connection.py:90` if we report an error to the user, we should cancel the underlying future otherwise a warning gets emitted, that there was a future exception never received. --------- Co-authored-by: Dmitry Gozman --- playwright/_impl/_browser_context.py | 2 +- playwright/_impl/_connection.py | 31 ++++++++++++++----- playwright/_impl/_helper.py | 12 ++++++- playwright/_impl/_page.py | 4 +-- .../test_browsercontext_request_intercept.py | 13 ++++++++ .../test_browsercontext_request_intercept.py | 13 ++++++++ 6 files changed, 64 insertions(+), 11 deletions(-) diff --git a/playwright/_impl/_browser_context.py b/playwright/_impl/_browser_context.py index 6ea934323..c52a134c8 100644 --- a/playwright/_impl/_browser_context.py +++ b/playwright/_impl/_browser_context.py @@ -123,7 +123,7 @@ def __init__( ) self._channel.on( "route", - lambda params: asyncio.create_task( + lambda params: self._loop.create_task( self._on_route( from_channel(params.get("route")), ) diff --git a/playwright/_impl/_connection.py b/playwright/_impl/_connection.py index 18b8b3423..ba6a8ba9f 100644 --- a/playwright/_impl/_connection.py +++ b/playwright/_impl/_connection.py @@ -53,6 +53,7 @@ def __init__(self, connection: "Connection", object: "ChannelOwner") -> None: self._connection = connection self._guid = object._guid self._object = object + self.on("error", lambda exc: self._connection._on_event_listener_error(exc)) async def send(self, method: str, params: Dict = None) -> Any: return await self._connection.wrap_api_call( @@ -77,13 +78,13 @@ async def inner_send( ) -> Any: if params is None: params = {} - callback = self._connection._send_message_to_server( - self._object, method, _filter_none(params) - ) if self._connection._error: error = self._connection._error self._connection._error = None raise error + callback = self._connection._send_message_to_server( + self._object, method, _filter_none(params) + ) done, _ = await asyncio.wait( { self._connection._transport.on_error_future, @@ -416,10 +417,22 @@ def dispatch(self, msg: ParsedMessagePayload) -> None: try: if self._is_sync: for listener in object._channel.listeners(method): + # Event handlers like route/locatorHandlerTriggered require us to perform async work. + # In order to report their potential errors to the user, we need to catch it and store it in the connection + def _done_callback(future: asyncio.Future) -> None: + exc = future.exception() + if exc: + self._on_event_listener_error(exc) + + def _listener_with_error_handler_attached(params: Any) -> None: + potential_future = listener(params) + if asyncio.isfuture(potential_future): + potential_future.add_done_callback(_done_callback) + # Each event handler is a potentilly blocking context, create a fiber for each # and switch to them in order, until they block inside and pass control to each # other and then eventually back to dispatcher as listener functions return. - g = EventGreenlet(listener) + g = EventGreenlet(_listener_with_error_handler_attached) if should_replace_guids_with_channels: g.switch(self._replace_guids_with_channels(params)) else: @@ -432,9 +445,13 @@ def dispatch(self, msg: ParsedMessagePayload) -> None: else: object._channel.emit(method, params) except BaseException as exc: - print("Error occurred in event listener", file=sys.stderr) - traceback.print_exc() - self._error = exc + self._on_event_listener_error(exc) + + def _on_event_listener_error(self, exc: BaseException) -> None: + print("Error occurred in event listener", file=sys.stderr) + traceback.print_exception(type(exc), exc, exc.__traceback__, file=sys.stderr) + # Save the error to throw at the next API call. This "replicates" unhandled rejection in Node.js. + self._error = exc def _create_remote_object( self, parent: ChannelOwner, type: str, guid: str, initializer: Dict diff --git a/playwright/_impl/_helper.py b/playwright/_impl/_helper.py index cd6a1d68c..20ab885f8 100644 --- a/playwright/_impl/_helper.py +++ b/playwright/_impl/_helper.py @@ -299,10 +299,20 @@ async def _handle_internal(self, route: "Route") -> bool: self._handled_count += 1 if self._is_sync: + handler_finished_future = route._loop.create_future() + + def _handler() -> None: + try: + self.handler(route, route.request) # type: ignore + handler_finished_future.set_result(None) + except Exception as e: + handler_finished_future.set_exception(e) + # As with event handlers, each route handler is a potentially blocking context # so it needs a fiber. - g = RouteGreenlet(lambda: self.handler(route, route.request)) # type: ignore + g = RouteGreenlet(_handler) g.switch() + await handler_finished_future else: coro_or_future = self.handler(route, route.request) # type: ignore if coro_or_future: diff --git a/playwright/_impl/_page.py b/playwright/_impl/_page.py index 9b0798bd5..db6cf13b8 100644 --- a/playwright/_impl/_page.py +++ b/playwright/_impl/_page.py @@ -180,13 +180,13 @@ def __init__( ) self._channel.on( "locatorHandlerTriggered", - lambda params: asyncio.create_task( + lambda params: self._loop.create_task( self._on_locator_handler_triggered(params["uid"]) ), ) self._channel.on( "route", - lambda params: asyncio.create_task( + lambda params: self._loop.create_task( self._on_route(from_channel(params["route"])) ), ) diff --git a/tests/async/test_browsercontext_request_intercept.py b/tests/async/test_browsercontext_request_intercept.py index ffb5103c1..1b4f57322 100644 --- a/tests/async/test_browsercontext_request_intercept.py +++ b/tests/async/test_browsercontext_request_intercept.py @@ -15,6 +15,7 @@ import asyncio from pathlib import Path +import pytest from twisted.web import http from playwright.async_api import BrowserContext, Page, Route @@ -179,3 +180,15 @@ async def test_should_give_access_to_the_intercepted_response_body( route.fulfill(response=response), eval_task, ) + + +async def test_should_show_exception_after_fulfill(page: Page, server: Server) -> None: + async def _handle(route: Route) -> None: + await route.continue_() + raise Exception("Exception text!?") + + await page.route("*/**", _handle) + await page.goto(server.EMPTY_PAGE) + # Any next API call should throw because handler did throw during previous goto() + with pytest.raises(Exception, match="Exception text!?"): + await page.goto(server.EMPTY_PAGE) diff --git a/tests/sync/test_browsercontext_request_intercept.py b/tests/sync/test_browsercontext_request_intercept.py index b136038ec..16cca8cfd 100644 --- a/tests/sync/test_browsercontext_request_intercept.py +++ b/tests/sync/test_browsercontext_request_intercept.py @@ -14,6 +14,7 @@ from pathlib import Path +import pytest from twisted.web import http from playwright.sync_api import BrowserContext, Page, Route @@ -121,3 +122,15 @@ def handle_route(route: Route) -> None: assert request.uri.decode() == "/title.html" original = (assetdir / "title.html").read_text() assert response.text() == original + + +def test_should_show_exception_after_fulfill(page: Page, server: Server) -> None: + def _handle(route: Route) -> None: + route.continue_() + raise Exception("Exception text!?") + + page.route("*/**", _handle) + page.goto(server.EMPTY_PAGE) + # Any next API call should throw because handler did throw during previous goto() + with pytest.raises(Exception, match="Exception text!?"): + page.goto(server.EMPTY_PAGE)