fix: report route handler errors to the user after they completed (microsoft#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 <[email protected]>

Author: mxschmitt

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")),
                 )

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

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:

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"]))
             ),
         )

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)

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)