fix(sync): raise instead of spinning after the driver exits

playwright/_impl/_disposable.py

@@ -12,14 +12,14 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-import asyncio
 import traceback
-from typing import Awaitable, Callable, Dict
+from typing import Any, Awaitable, Callable, Coroutine, Dict
 
 import greenlet
 
 from playwright._impl._connection import ChannelOwner, _capture_stack_trace
 from playwright._impl._errors import Error, is_target_closed_error
+from playwright._impl._greenlets import connection_closed_error, wait_for_future
 
 
 class Disposable(ChannelOwner):
@@ -70,23 +70,24 @@ def __enter__(self) -> "DisposableStub":
     def __exit__(self, *args: object) -> None:
         self._sync(self.dispose())
 
-    def _sync(self, coro: object) -> object:
+    def _sync(self, coro: Coroutine[Any, Any, Any]) -> object:
         __tracebackhide__ = True
         if self._loop.is_closed():
-            coro.close()  # type: ignore
+            coro.close()
             raise Error("Event loop is closed! Is Playwright already stopped?")
+        if self._dispatcher_fiber.dead:
+            coro.close()
+            raise connection_closed_error()
         g_self = greenlet.getcurrent()
-        task = self._loop.create_task(coro)  # type: ignore
+        task = self._loop.create_task(coro)
         setattr(
             task,
             "__pw_stack__",
             _capture_stack_trace(),
         )
         setattr(task, "__pw_stack_trace__", traceback.extract_stack(limit=10))
         task.add_done_callback(lambda _: g_self.switch())
-        while not task.done():
-            self._dispatcher_fiber.switch()  # type: ignore
-        asyncio._set_running_loop(self._loop)
+        wait_for_future(self._loop, self._dispatcher_fiber, task)
         return task.result()
 
     async def close(self) -> None:

playwright/_impl/_greenlets.py

@@ -11,11 +11,14 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
+import asyncio
 import os
 from typing import Tuple
 
 import greenlet
 
+from playwright._impl._errors import TargetClosedError
+
 
 def _greenlet_trace_callback(
     event: str, args: Tuple[greenlet.greenlet, greenlet.greenlet]
@@ -47,3 +50,26 @@ def __str__(self) -> str:
 class EventGreenlet(greenlet.greenlet):
     def __str__(self) -> str:
         return "<EventGreenlet>"
+
+
+def connection_closed_error() -> TargetClosedError:
+    return TargetClosedError("Playwright connection closed")
+
+
+def wait_for_future(
+    loop: asyncio.AbstractEventLoop,
+    dispatcher_fiber: greenlet.greenlet,
+    future: "asyncio.Future",
+) -> None:
+    __tracebackhide__ = True
+    while not future.done():
+        # The dispatcher fiber exits once the connection to the driver ends, e.g.
+        # when the driver process dies. Nothing can settle the future after that,
+        # and switching to a dead greenlet returns right away, so we would spin.
+        if dispatcher_fiber.dead:
+            future.cancel()
+            raise connection_closed_error()
+        dispatcher_fiber.switch()
+    # The loop is only running for as long as the dispatcher fiber is alive.
+    if not dispatcher_fiber.dead:
+        asyncio._set_running_loop(loop)

playwright/_impl/_sync_base.py

@@ -32,6 +32,7 @@
 import greenlet
 
 from playwright._impl._connection import _capture_stack_trace
+from playwright._impl._greenlets import connection_closed_error, wait_for_future
 from playwright._impl._helper import Error
 from playwright._impl._impl_to_api_mapping import ImplToApiMapping, ImplWrapper
 
@@ -51,9 +52,9 @@ def __init__(self, sync_base: "SyncBase", future: "asyncio.Future[T]") -> None:
 
     @property
     def value(self) -> T:
-        while not self._future.done():
-            self._sync_base._dispatcher_fiber.switch()
-        asyncio._set_running_loop(self._sync_base._loop)
+        wait_for_future(
+            self._sync_base._loop, self._sync_base._dispatcher_fiber, self._future
+        )
         exception = self._future.exception()
         if exception:
             raise exception
@@ -102,16 +103,17 @@ def _sync(
         if self._loop.is_closed():
             coro.close()
             raise Error("Event loop is closed! Is Playwright already stopped?")
+        if self._dispatcher_fiber.dead:
+            coro.close()
+            raise connection_closed_error()
 
         g_self = greenlet.getcurrent()
         task: asyncio.tasks.Task[Any] = self._loop.create_task(coro)
         setattr(task, "__pw_stack__", _capture_stack_trace())
         setattr(task, "__pw_stack_trace__", traceback.extract_stack(limit=10))
 
         task.add_done_callback(lambda _: g_self.switch())
-        while not task.done():
-            self._dispatcher_fiber.switch()
-        asyncio._set_running_loop(self._loop)
+        wait_for_future(self._loop, self._dispatcher_fiber, task)
         return task.result()
 
     def _wrap_handler(

tests/sync/test_sync.py

@@ -399,6 +399,55 @@ def test_should_not_orphan_callback_on_non_serializable_params(
     assert "Future exception was never retrieved" not in result.stderr
 
 
+def test_sync_calls_should_raise_after_driver_exit(
+    browser_name: str,
+    launch_arguments: Dict[str, Any],
+    tmp_path: Path,
+) -> None:
+    # Regression test for https://github.com/microsoft/playwright-python/pull/3187.
+    # Run in a subprocess with a timeout, the calls used to spin forever instead of raising.
+    script = tmp_path / "driver_exit.py"
+    script.write_text(
+        textwrap.dedent(
+            f"""
+            from playwright.sync_api import sync_playwright
+
+
+            def error_of(callback):
+                try:
+                    callback()
+                except Exception as e:
+                    return str(e)
+                return ""
+
+
+            with sync_playwright() as p:
+                browser = p[{browser_name!r}].launch(**{launch_arguments!r})
+                page = browser.new_page()
+                route = page.route("**/*", lambda route: route.continue_())
+
+                def expect_event():
+                    with page.expect_event("console", timeout=0):
+                        p._impl_obj._connection._transport._proc.kill()
+
+                def dispose_route():
+                    with route:
+                        pass
+
+                for callback in [expect_event, page.title, dispose_route, browser.close]:
+                    assert "Playwright connection closed" in error_of(callback), callback
+            """
+        )
+    )
+    result = subprocess.run(
+        [sys.executable, str(script)],
+        capture_output=True,
+        text=True,
+        timeout=60,
+    )
+    assert result.returncode == 0, result.stderr
+
+
 def test_click_should_accept_timedelta_for_timeout(page: Page) -> None:
     with pytest.raises(TimeoutError, match="Timeout 1ms exceeded"):
         page.click("does-not-exist", timeout=timedelta(milliseconds=1))