Skip to content

fix(sync): deactivate resume callbacks when waits end - #3311

Open
microsoft-playwright-automation[bot] wants to merge 1 commit into
mainfrom
triage/3307/41557163294-1
Open

microsoft-playwright-automation[bot] wants to merge 1 commit into
mainfrom
triage/3307/41557163294-1

Conversation

@microsoft-playwright-automation

Copy link
Copy Markdown
Contributor
  • A wait on an already completed future queues a resume callback but returns without running it. That callback can then resume user code during shutdown, while the dispatcher's event loop is still running.
  • Make the resume callback inactive when its wait ends, and remove it from the future. The flag also handles callbacks already queued by asyncio, which remove_done_callback() cannot remove.
  • Extend the existing subprocess test for explicit Playwright shutdown with a completed console-event wait. It fails without the fix and passes with it on Chromium, Firefox and WebKit.

Verified on Linux with Python 3.12.15: sync shutdown and console checks under xdist (125 passed, one existing upstream skip); async context-manager and console checks (62 passed, the same skip). mypy playwright and ./scripts/lint.sh pass.

Fixes #3307

Triaged on Linux by the Playwright bot - agent run

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix in _greenlets.py is right. Two changes:

  1. The test fails on the stable channels, see the inline comment: use a predicate.
  2. In the except branch of wait_for_future, remove the callback before future.cancel(), as before. A cancelled plain future otherwise queues an inert callback.

Ignore the macOS WebGL and the Windows WebKit screencast failures; they are not from this change.

Comment thread tests/sync/python/test_sync.py Outdated
context = browser.new_context()
page = context.new_page()
page.goto(empty_page)
with page.expect_console_message() as console_info:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails on the Linux chrome, Windows chrome and Windows msedge jobs: the child asserts at console_info.value.text == "hello" on every attempt. Branded Chrome and Edge emit another console message first, and without a predicate the block takes the first message. Match the message:

Suggested change
with page.expect_console_message() as console_info:
with page.expect_console_message(lambda m: m.text == "hello") as console_info:

The predicate runs inside the event dispatch, so the future is still done before __exit__ waits on it, and the test still covers the bug.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the predicate so the wait matches only hello. The regression still fails without the fix on Chromium, Firefox and WebKit, and passes with it on all three, plus stable Chrome and Edge on Linux.

Revised on Linux by the Playwright bot - agent run

@microsoft-playwright-automation

Copy link
Copy Markdown
Contributor Author

Hi, I'm the Playwright bot. I addressed both review requests.

The shutdown test now matches the hello console message with a predicate. The exception branch removes the resume callback before future.cancel(). The unrelated macOS WebGL and Windows WebKit screencast failures were left unchanged.

On Linux with Python 3.12.15, the updated regression fails without the fix on Chromium, Firefox and WebKit, and passes with it on all three, plus stable Chrome and Edge. The neighbouring sync checks pass (125 passed, 1 upstream skip); the async checks pass (65 passed, 4 expected skips). mypy playwright and ./scripts/lint.sh pass. Windows and macOS were not tested on this runner.

Revised on Linux by the Playwright bot - agent run

@pavelfeldman
Pavel Feldman (pavelfeldman) marked this pull request as ready for review November 3, 2026 22:52

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Sync API — pw.stop() raises "This event loop is already running" at teardown after 1.64 sync-dispatcher rework (#3230)

1 participant