[BUG] Sync API functions expecting to be executed in a asyncio task

[vEpiphyte]

Context:

• Playwright Version: 1.21.0
• Operating System: Linux / Ubuntu
• Python version: 3.8.x
• Browser: Chromium (presumably All would be affected)
• Extra: Unit test execution

Code Snippet

I don't have a small reproducible snippet, but could come up with one if needed. Here is a example traceback that was encountered when running tests.

__________________________________________________ SomeTest.test_stuff_async ___________________________________________________
../../.pyenv/versions/3.8.10/envs/project3810/lib/python3.8/site-packages/package00/tests/test_playwright/common.py:89: in setUp
    self.page.set_default_timeout(20000)
../../.pyenv/versions/3.8.10/envs/project3810/lib/python3.8/site-packages/playwright/async_api/_generated.py:6609: in set_default_timeout
    self._impl_obj.set_default_timeout(timeout=timeout)
../../.pyenv/versions/3.8.10/envs/project3810/lib/python3.8/site-packages/playwright/_impl/_page.py:347: in set_default_timeout
    self._channel.send_no_reply("setDefaultTimeoutNoReply", dict(timeout=timeout))
../../.pyenv/versions/3.8.10/envs/project3810/lib/python3.8/site-packages/playwright/_impl/_connection.py:80: in send_no_reply
    self._connection._send_message_to_server(self._guid, method, params)
../../.pyenv/versions/3.8.10/envs/project3810/lib/python3.8/site-packages/playwright/_impl/_connection.py:227: in _send_message_to_server
    callback = ProtocolCallback(self._loop)
../../.pyenv/versions/3.8.10/envs/project3810/lib/python3.8/site-packages/playwright/_impl/_connection.py:128: in __init__
    current_task = asyncio.current_task()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

loop = None

    def current_task(loop=None):
        """Return a currently executed task."""
        if loop is None:
>           loop = events.get_running_loop()
E           RuntimeError: no running event loop

../../.pyenv/versions/3.8.10/lib/python3.8/asyncio/tasks.py:37: RuntimeError

Describe the bug

With the changes in #1236 the ProtocolCallback.init method now expects to always be called from inside of an async task [due to the use of asyncio.current_task()].

There are a handful of APIs which are not async functions and previously worked when executed outside of an async task, such as Page.set_default_timeout which can be found here https://github.com/microsoft/playwright-python/blob/v1.21.0/playwright/_impl/_page.py#L345

My coworkers and I stumbled across this due to unit test failures occurring inside of Python Unittest.TestCase.setUp() methods where the playwright context and new page where explicitly being handled by an ioloop constructed for the test, but the set_default_timeout() call was being called directly from the mainthread. Since there was no running ioloop in the main thread, the use of asyncio.current_task() raises an exception.

I'm not certain if there is a direct fix here other than to "Do all playwright related activities in the thread it was made" which is fine for us, but wanted to record this in the event that other users stumbled across this issue, since code which previously worked with 1.20.x could now break.

[mxschmitt]

Hey! Having a repro for it would be very helpful. I tried the following but it was working for me:

from playwright.sync_api import sync_playwright

with sync_playwright() as p:
    browser = p.chromium.launch()
    page = browser.new_page()
    page.goto("http://playwright.dev")
    page.set_default_timeout(32000)
    print(page.title())
    browser.close()

If something broke due the last update, then we'll definitely consider reverting or fixing it!

[vEpiphyte]

@mxschmitt Sorry - this isn't the smallest repro, but its a bit of an amalgamation of all our code which got us into this place.

# Repro case for https://github.com/microsoft/playwright-python/issues/1263
import re
import os
import asyncio
import threading
import inspect

import copy
import logging
import contextlib

import unittest

from playwright.async_api import async_playwright, Browser, BrowserContext, Page


logger = logging.getLogger(__name__)

_glob_loop = None
_glob_thrd = None

def initloop():
    # Build an ioloop running in a thread or return the global loop.
    global _glob_loop
    global _glob_thrd

    # if there's no global loop....
    if _glob_loop is None:

        # check if it's us....
        try:
            _glob_loop = asyncio.get_running_loop()
            # if we get here, it's us!
            _glob_thrd = threading.currentThread()

        except RuntimeError:
            _glob_loop = asyncio.new_event_loop()

            _glob_thrd = threading.Thread(target=_glob_loop.run_forever, name='SynLoop')
            _glob_thrd.setDaemon(True)
            _glob_thrd.start()

    return _glob_loop

def iAmLoop():
    initloop()
    return threading.currentThread() == _glob_thrd

def sync(coro, timeout=None):
    '''
    Schedule a coroutine to run on the global loop and return it's result.

    This API is thread safe and should only be called by non-loop threads.
    '''
    loop = initloop()
    return asyncio.run_coroutine_threadsafe(coro, loop).result(timeout)

def synchelp(f):
    '''
    The synchelp decorator allows the transparent execution of
    a coroutine using the global loop from a thread other than
    the event loop.  In both use cases, the actual work is done
    by the global event loop.
    '''
    def wrap(*args, **kwargs):

        coro = f(*args, **kwargs)

        if not iAmLoop():
            return sync(coro)

        return coro

    return wrap


PWDEBUG = os.environ.get('PWDEBUG') is not None

@contextlib.asynccontextmanager
async def getPlaywright():
    async with async_playwright() as p:
        browser = await p.chromium.launch()

        yield browser

class PlaywrightTest(unittest.TestCase):

    browser: Browser
    context: BrowserContext
    page: Page

    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)

        for s in dir(self):
            attr = getattr(self, s, None)
            # If s is an instance method and starts with 'test_', synchelp wrap it to ensure that it is
            # executed in the ioloop context.
            if inspect.iscoroutinefunction(attr) and s.startswith('test_') and inspect.ismethod(attr):
                setattr(self, s, synchelp(attr))

    @classmethod
    def setUpClass(cls):
        # Setup the browser only once because it's expensive
        cls._acm_playwright = getPlaywright()
        cls.browser = sync(cls._acm_playwright.__aenter__())

    @classmethod
    def tearDownClass(cls):
        sync(cls.browser.close())

    def setUp(self) -> None:

        # setup playwright context and page
        self.context = sync(self._new_context())
        sync(self.context.tracing.start(screenshots=True, snapshots=True))
        self.page = sync(self.context.new_page())
        sync(self.page.set_viewport_size({'width': 1920, 'height': 1440}))

        if not PWDEBUG:
            self.page.set_default_timeout(20000)  # <------- This is a call from MainThread into a object created by SynLoop

    def tearDown(self):
        sync(self.page.close())

    async def _new_context(self):
        return await self.browser.new_context(
            ignore_https_errors=True,
            accept_downloads=True,
            viewport={'width': 1920, 'height': 1440}
        )

    async def test_demo(self):
        await asyncio.sleep(0)
        self.assertTrue(True)

For tests, we're constructing the ioloop in its own thread, and running async setup code through async/sync helper functions. This ends up creating an async_playwright instance from SynLoop, and then accessing that object from MainThread to call the timeout function. This is violating playwright-pythons documented not thread-safe behavior.

Running this with pytest python -m pytest test_playwright_repro.py will throw the RuntimeError; PWDEBUG=1 python -m pytest test_playwright_repro.py will not.

[mxschmitt]

Yeah its a big repro - before I'm digging into it, what is the reason you are using the async API and manually wrapping it to make it sync instead of using the sync API?

[vEpiphyte]

@mxschmitt - all of our tests are run in an async context since we're testing an async application with multiple components and cannot have anything blocking. We're using the class setup to create the browser (a heavy operation) and then per test setup methods to create our application contexts and the new player context. Much of this setup code is similar to how playwrights SyncBase operates (https://github.com/microsoft/playwright-python/blob/v1.21.0/playwright/_impl/_sync_base.py#L68) to move async calls in the generated sync api code to be executed in the ioloop.

There is a bit of an "async api" design here, which is something we've encountered with porting large applications from threading models to async models. The async-ification of things is generally contagious, and almost always better to do up front instead of needing to change library interfaces when you realize the guts of something must be async, affecting all the callers suddenly needing to become async as well.

Reviewing the playwright generation code [and assuming my understanding is correct], if _impl.page.set_default_timeout and set_default_navigation_timeout were, for example, marked as async functions [even though they aren't awaiting anything themselves] it would cause the async_playwright versions of those to require them to be properly awaited by callers, and the sync version to properly push the call through the SyncBase helper. If I had to take a guess, no one else has bumped into this since in most sync use cases create / teardown an ioloop as needed and those timeout calls are main inside of that context manager.

[rwoll]

To use Playwright as async, we recommend using the default Playwright async impl. We do not recommend wrapping the sync version again as it is an unsupported mode of operation. Thanks!