fix: asyncio.create_task race issue in page and brower_context

[x0day]

we should consider ._channel.send have race condition issue with page close event in some Page's method.

Page._update_interception_patterns is just a situation i encountered.

This problem is triggered when the Page.unroute method is called when the page is about to close.

[mxschmitt]

I think we can simplify this PR by mirroring our upstream logic:

https://github.com/microsoft/playwright/blob/d7b3836752fbb996a4eb7dd6e36c3970912ac2e4/packages/playwright-core/src/client/page.ts#L193

This means inside

playwright-python/playwright/_impl/_page.py

Line 260 in 9717d13

self._connection.wrap_api_call(

we try-catch basically inside the lambda.

This means inside

[x0day]

@mxschmitt There are too many calls to asyncio.create_task in the implementation of python, and these calls may cause race issues, such as the processing of dialog. I want to re-check the implementation of this part of the page and unify it with reference to the implementation of JavaScript.

[x0day]

@mxschmitt done. please check again.

[mxschmitt]

the bots seem all unhappy and timing out.

[x0day]

I'm on vacation these days and will check again later.

[x0day]

@mxschmitt ready for review, this PR contains:

1. fix asyncio.create_task fire-and-forget usage include keep reference to the task for gc issues like Task was destroyed but it is pending!.
2. we can't unify it like javascript because sync api is broken when listener is a coroutine.

[x0day]

@mxschmitt any thoughts about this? should I split the patch for AsyncIOEventEmitter on another PR or something else?

[mxschmitt]

Sorry for being slow on this, the last few weeks were busy on my end and we landed a bunch of related changes in the 1.34 release, so we tried not put another change which might introduce regressions into it.

I will get back to you in a few days. Would it be possible to add a test/s for it?

[mxschmitt]

I was talking with my colleagues about it and it still seems, instead of introducing so much complexity, it would be much nicer to do it as the way I explained in #1864 (review). This would fix the actual problem you reported in the first place. Appreciate the efforts, thanks!

[x0day]

@mxschmitt sorry for delay, it can't try await ... /except in listener. if we change handler to async def. it will broken sync api here.

playwright-python/playwright/_impl/_connection.py

Lines 409 to 418 in e6a7a37

	if self._is_sync:
	for listener in object._channel.listeners(method):
	# 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 = greenlet(listener)
	if should_replace_guids_with_channels:
	g.switch(self._replace_guids_with_channels(params))
	else:
	g.switch(params)

probably should add _emit_sync in base channel, and replace asyncio.create_task everywhere. this is the correct way for use asyncio.create_task in sync method.
and remove the fix for pyee.AsyncIOEventEmitter, wait for upstream vote or patch. jfhbrook/pyee#120

    def _emit_sync(self, coro: Coroutine, ignore_errors: bool = True) -> None:
        fut = asyncio.ensure_future(coro, loop=self._loop)

        def cb(f: asyncio.Task) -> None:
            self._running_tasks.remove(f)
            if f.cancelled():
                return

            exc: Optional[BaseException] = f.exception()
            if exc and not ignore_errors:
                self.emit("error", exc)

        self._running_tasks.add(fut)
        fut.add_done_callback(cb)

Reference:

• asyncio.create_task() documentation should mention user needs to keep reference to the task python/cpython#88831
• https://stackoverflow.com/questions/71938799/python-asyncio-create-task-really-need-to-keep-a-reference
• https://docs.python.org/3/library/asyncio-task.html

Important Save a reference to the result of this function, to avoid a task disappearing mid-execution. The event loop only keeps weak references to tasks. A task that isn’t referenced elsewhere may get garbage collected at any time, even before it’s done. For reliable “fire-and-forget” background tasks, gather them in a collection.

[A1exKH]

Code LGTM.
Please resolve conflicts for this PR.

[mxschmitt]

This PR looks stale and doesn't link a specific GitHub issue which it fixes. I'll close it for now but don't hesitate to re-create it with a test and linked issue. Thanks for your collaboration!