diff --git a/CLAUDE.md b/CLAUDE.md index ce4ec7c07..8c9e32bfb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -55,3 +55,40 @@ It documents the full process: the upstream commit-range diff over `docs/src/api - New public methods on impl classes need a sync test mirror under `tests/sync/`. - Keep `expected_api_mismatch.txt` minimal — every entry needs a one-line rationale comment above it. - Prefer `locals_to_params(locals())` for forwarding optional kwargs to channel sends, matching the rest of the codebase. + +## Commit Convention + +Before committing, run `mypy playwright` and fix errors. + +Semantic commit messages: `label(scope): description` + +Labels: `fix`, `feat`, `chore`, `docs`, `test`, `devops` + +```bash +git checkout -b fix-12345 +# ... make changes ... +git add +git commit -m "$(cat <<'EOF' +fix(asyncio): do not deadlock in atexit handler + +Fixes: https://github.com/microsoft/playwright-python/issues/12345 +EOF +)" +git push origin fix-12345 +gh pr create --repo microsoft/playwright-python --head username:fix-12345 \ + --title "fix(asyncio): do not deadlock in atexit handler" \ + --body "$(cat <<'EOF' +## Summary +- + +Fixes https://github.com/microsoft/playwright-python/issues/12345 +EOF +)" +``` + +Never add Co-Authored-By agents in commit message. +Never add "Generated with" in commit message. +Never add test plan to PR description. Keep PR description short — a few bullet points at most. +Branch naming for issue fixes: `fix-` + +**Never `git push` without an explicit instruction to push.** Applies even when a PR is already open for the branch — additional commits are immediately visible to reviewers. Commit locally, report what was committed, and wait. Only push when the user's message contains "push", "upload", "create PR", "ship it", or equivalent. diff --git a/local-requirements.txt b/local-requirements.txt index 8a72b5745..5735ae2a0 100644 --- a/local-requirements.txt +++ b/local-requirements.txt @@ -1,3 +1,4 @@ +asyncio-atexit==1.0.1 autobahn==23.1.2 black==25.1.0 build==1.3.0 diff --git a/playwright/_impl/_transport.py b/playwright/_impl/_transport.py index 2ca84d459..3cc029e18 100644 --- a/playwright/_impl/_transport.py +++ b/playwright/_impl/_transport.py @@ -137,38 +137,44 @@ async def connect(self) -> None: async def run(self) -> None: assert self._proc.stdout assert self._proc.stdin - while not self._stopped: - try: - buffer = await self._proc.stdout.readexactly(4) - if self._stopped: - break - length = int.from_bytes(buffer, byteorder="little", signed=False) - buffer = bytes(0) - while length: - to_read = min(length, 32768) - data = await self._proc.stdout.readexactly(to_read) + try: + while not self._stopped: + try: + buffer = await self._proc.stdout.readexactly(4) + if self._stopped: + break + length = int.from_bytes(buffer, byteorder="little", signed=False) + buffer = bytes(0) + while length: + to_read = min(length, 32768) + data = await self._proc.stdout.readexactly(to_read) + if self._stopped: + break + length -= to_read + if len(buffer): + buffer = buffer + data + else: + buffer = data if self._stopped: break - length -= to_read - if len(buffer): - buffer = buffer + data - else: - buffer = data - if self._stopped: - break - obj = self.deserialize_message(buffer) - self.on_message(obj) - except asyncio.IncompleteReadError: - if not self._stopped: - self.on_error_future.set_exception( - Exception("Connection closed while reading from the driver") - ) - break - await asyncio.sleep(0) - - await self._proc.communicate() - self._stopped_future.set_result(None) + obj = self.deserialize_message(buffer) + self.on_message(obj) + except asyncio.IncompleteReadError: + if not self._stopped: + self.on_error_future.set_exception( + Exception("Connection closed while reading from the driver") + ) + break + await asyncio.sleep(0) + + await self._proc.communicate() + finally: + # Release waiters on wait_until_stopped() even if this task was + # cancelled before reaching the end (e.g. by asyncio.run()'s + # task-cancellation phase that runs before asyncio-atexit hooks). + if not self._stopped_future.done(): + self._stopped_future.set_result(None) def send(self, message: Dict) -> None: assert self._output diff --git a/tests/async/test_asyncio.py b/tests/async/test_asyncio.py index 971c65473..c78648430 100644 --- a/tests/async/test_asyncio.py +++ b/tests/async/test_asyncio.py @@ -13,7 +13,10 @@ # limitations under the License. import asyncio import gc +import subprocess import sys +import textwrap +from pathlib import Path from typing import Dict import pytest @@ -89,6 +92,46 @@ async def raise_exception() -> None: assert await page.evaluate("() => 11 * 11") == 121 +def test_stop_does_not_deadlock_with_asyncio_atexit(tmp_path: Path) -> None: + # Regression test for https://github.com/microsoft/playwright-python/issues/3004. + # asyncio.run() cancels all remaining tasks (including transport.run()) before + # calling loop.close(). asyncio-atexit hooks loop.close() to run async cleanup, + # so awaiting playwright.stop() at that point used to deadlock on a future that + # the (already cancelled) run task would never set. + script = tmp_path / "atexit_stop.py" + script.write_text( + textwrap.dedent( + """ + import asyncio + + import asyncio_atexit + from playwright.async_api import async_playwright + + + async def main(): + pw = await async_playwright().start() + asyncio_atexit.register(lambda: stop(pw)) + + + async def stop(pw): + await pw.stop() + print("STOPPED", flush=True) + + + asyncio.run(main()) + """ + ) + ) + result = subprocess.run( + [sys.executable, str(script)], + capture_output=True, + text=True, + timeout=30, + ) + assert result.returncode == 0, result.stderr + assert "STOPPED" in result.stdout + + async def test_should_return_proper_api_name_on_error(page: Page) -> None: try: await page.evaluate("does_not_exist")