From 2951467f5125aaa6e446697096883b3f989d63db Mon Sep 17 00:00:00 2001 From: dm Date: Mon, 26 Apr 2021 12:02:07 +0800 Subject: [PATCH 01/10] fix(transport): handle connection error correctly (#650) --- playwright/_impl/_transport.py | 29 ++++++++++++++++-------- playwright/async_api/_context_manager.py | 10 ++++++-- 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/playwright/_impl/_transport.py b/playwright/_impl/_transport.py index fb20f3392..d2f567c77 100644 --- a/playwright/_impl/_transport.py +++ b/playwright/_impl/_transport.py @@ -43,6 +43,7 @@ def _get_stderr_fileno() -> Optional[int]: class Transport(ABC): def __init__(self) -> None: self.on_message = lambda _: None + self.on_error_future: asyncio.Future = asyncio.Future() @abstractmethod def request_stop(self) -> None: @@ -57,7 +58,6 @@ async def wait_until_stopped(self) -> None: async def run(self) -> None: self._loop = asyncio.get_running_loop() - self.on_error_future: asyncio.Future = asyncio.Future() @abstractmethod def send(self, message: Dict) -> None: @@ -96,14 +96,19 @@ async def run(self) -> None: await super().run() self._stopped_future: asyncio.Future = asyncio.Future() - self._proc = proc = await asyncio.create_subprocess_exec( - str(self._driver_executable), - "run-driver", - stdin=asyncio.subprocess.PIPE, - stdout=asyncio.subprocess.PIPE, - stderr=_get_stderr_fileno(), - limit=32768, - ) + try: + self._proc = proc = await asyncio.create_subprocess_exec( + str(self._driver_executable), + "run-driver", + stdin=asyncio.subprocess.PIPE, + stdout=asyncio.subprocess.PIPE, + stderr=_get_stderr_fileno(), + limit=32768, + ) + except FileNotFoundError: + self.on_error_future.set_exception(Error("playwright's driver is not found")) + return + assert proc.stdout assert proc.stdin self._output = proc.stdin @@ -163,7 +168,11 @@ async def run(self) -> None: if self.timeout is not None: options["close_timeout"] = self.timeout / 1000 options["ping_timeout"] = self.timeout / 1000 - self._connection = await websockets.connect(self.ws_endpoint, **options) + try: + self._connection = await websockets.connect(self.ws_endpoint, **options) + except Exception as err: + self.on_error_future.set_exception(Error(f"playwright's websocket endpoint connection error: {err}")) + return while not self._stopped: try: diff --git a/playwright/async_api/_context_manager.py b/playwright/async_api/_context_manager.py index 51800dd48..8705ec3d8 100644 --- a/playwright/async_api/_context_manager.py +++ b/playwright/async_api/_context_manager.py @@ -33,9 +33,15 @@ async def __aenter__(self) -> AsyncPlaywright: loop = asyncio.get_running_loop() self._connection._loop = loop loop.create_task(self._connection.run()) - playwright = AsyncPlaywright( - await self._connection.wait_for_object_with_known_name("Playwright") + done, pending = await asyncio.wait( + { + self._connection._transport.on_error_future, # type: ignore + self._connection.wait_for_object_with_known_name("Playwright"), + }, + return_when=asyncio.FIRST_COMPLETED, ) + obj = next(iter(done)).result() + playwright = AsyncPlaywright(obj) playwright.stop = self.__aexit__ # type: ignore return playwright From f6b4b106d79dd162c98d29b32d393fdfb1fefad9 Mon Sep 17 00:00:00 2001 From: dm Date: Mon, 26 Apr 2021 18:27:14 +0800 Subject: [PATCH 02/10] fix(transport): error message when driver not found --- playwright/_impl/_transport.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/playwright/_impl/_transport.py b/playwright/_impl/_transport.py index d2f567c77..c2767fb83 100644 --- a/playwright/_impl/_transport.py +++ b/playwright/_impl/_transport.py @@ -106,7 +106,11 @@ async def run(self) -> None: limit=32768, ) except FileNotFoundError: - self.on_error_future.set_exception(Error("playwright's driver is not found")) + self.on_error_future.set_exception(Error( + "playwright's driver is not found, You can read the contributing guide " + "for some guidance on how to get everything setup for working on the code " + "https://github.com/microsoft/playwright-python/blob/master/CONTRIBUTING.md" + )) return assert proc.stdout From cdf4bff120510439dc3158b6cffa86f895cdca33 Mon Sep 17 00:00:00 2001 From: dm Date: Mon, 26 Apr 2021 18:42:58 +0800 Subject: [PATCH 03/10] fix: lint error --- playwright/_impl/_transport.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/playwright/_impl/_transport.py b/playwright/_impl/_transport.py index c2767fb83..517555c4a 100644 --- a/playwright/_impl/_transport.py +++ b/playwright/_impl/_transport.py @@ -106,11 +106,13 @@ async def run(self) -> None: limit=32768, ) except FileNotFoundError: - self.on_error_future.set_exception(Error( - "playwright's driver is not found, You can read the contributing guide " - "for some guidance on how to get everything setup for working on the code " - "https://github.com/microsoft/playwright-python/blob/master/CONTRIBUTING.md" - )) + self.on_error_future.set_exception( + Error( + "playwright's driver is not found, You can read the contributing guide " + "for some guidance on how to get everything setup for working on the code " + "https://github.com/microsoft/playwright-python/blob/master/CONTRIBUTING.md" + ) + ) return assert proc.stdout @@ -175,7 +177,9 @@ async def run(self) -> None: try: self._connection = await websockets.connect(self.ws_endpoint, **options) except Exception as err: - self.on_error_future.set_exception(Error(f"playwright's websocket endpoint connection error: {err}")) + self.on_error_future.set_exception( + Error(f"playwright's websocket endpoint connection error: {err}") + ) return while not self._stopped: From 90831e0c1a89a8343c6f4700ce4f7bcf53fa1b05 Mon Sep 17 00:00:00 2001 From: dm Date: Tue, 27 Apr 2021 10:28:30 +0800 Subject: [PATCH 04/10] fix: cancel the obj when error --- playwright/async_api/_context_manager.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/playwright/async_api/_context_manager.py b/playwright/async_api/_context_manager.py index 8705ec3d8..68a9a986c 100644 --- a/playwright/async_api/_context_manager.py +++ b/playwright/async_api/_context_manager.py @@ -33,15 +33,20 @@ async def __aenter__(self) -> AsyncPlaywright: loop = asyncio.get_running_loop() self._connection._loop = loop loop.create_task(self._connection.run()) + obj = asyncio.ensure_future( + self._connection.wait_for_object_with_known_name("Playwright") + ) done, pending = await asyncio.wait( { + obj, self._connection._transport.on_error_future, # type: ignore - self._connection.wait_for_object_with_known_name("Playwright"), }, return_when=asyncio.FIRST_COMPLETED, ) + if not obj.done(): + obj.cancel() obj = next(iter(done)).result() - playwright = AsyncPlaywright(obj) + playwright = AsyncPlaywright(obj) # type: ignore playwright.stop = self.__aexit__ # type: ignore return playwright From 99289bfaac978fc9fddc2138744db77c411d5c61 Mon Sep 17 00:00:00 2001 From: dm Date: Tue, 27 Apr 2021 10:28:46 +0800 Subject: [PATCH 05/10] fix: cancel the obj when error --- playwright/_impl/_browser_type.py | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/playwright/_impl/_browser_type.py b/playwright/_impl/_browser_type.py index b7b8f0ad8..613f3ff91 100644 --- a/playwright/_impl/_browser_type.py +++ b/playwright/_impl/_browser_type.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import asyncio from pathlib import Path from typing import Dict, List, Optional, Union, cast @@ -174,8 +175,20 @@ async def connect( connection._is_sync = self._connection._is_sync connection._loop = self._connection._loop connection._loop.create_task(connection.run()) + obj = asyncio.ensure_future( + connection.wait_for_object_with_known_name("Playwright") + ) + done, pending = await asyncio.wait( + { + obj, + connection._transport.on_error_future, # type: ignore + }, + return_when=asyncio.FIRST_COMPLETED, + ) + if not obj.done(): + obj.cancel() + playwright = next(iter(done)).result() self._connection._child_ws_connections.append(connection) - playwright = await connection.wait_for_object_with_known_name("Playwright") pre_launched_browser = playwright._initializer.get("preLaunchedBrowser") assert pre_launched_browser browser = cast(Browser, from_channel(pre_launched_browser)) From 3ef700901b5c2d37bbfa228915c781a84715187b Mon Sep 17 00:00:00 2001 From: dm Date: Tue, 27 Apr 2021 10:29:05 +0800 Subject: [PATCH 06/10] feat: test for connection error --- tests/async/test_browsertype_connect.py | 121 +----------------------- tests/sync/test_browsertype_connect.py | 10 ++ 2 files changed, 13 insertions(+), 118 deletions(-) diff --git a/tests/async/test_browsertype_connect.py b/tests/async/test_browsertype_connect.py index 842b6c1ee..1ffaa6fc3 100644 --- a/tests/async/test_browsertype_connect.py +++ b/tests/async/test_browsertype_connect.py @@ -58,127 +58,12 @@ async def test_browser_type_connect_should_be_able_to_connect_two_browsers_at_th # original browser should still work assert await page2.evaluate("7 * 6") == 42 - await browser2.close() - -async def test_browser_type_connect_disconnected_event_should_be_emitted_when_browser_is_closed_or_server_is_closed( - browser_type: BrowserType, launch_server -): - # Launch another server to not affect other tests. - remote = launch_server() - - browser1 = await browser_type.connect(remote.ws_endpoint) - browser2 = await browser_type.connect(remote.ws_endpoint) - - disconnected1 = [] - disconnected2 = [] - browser1.on("disconnected", lambda: disconnected1.append(True)) - browser2.on("disconnected", lambda: disconnected2.append(True)) - - page2 = await browser2.new_page() - - await browser1.close() - assert len(disconnected1) == 1 - assert len(disconnected2) == 0 - - remote.kill() - assert len(disconnected1) == 1 - - with pytest.raises(Error): - # Tickle connection so that it gets a chance to dispatch disconnect event. - await page2.title() - assert len(disconnected2) == 1 - - -async def test_browser_type_disconnected_event_should_have_browser_as_argument( - browser_type: BrowserType, launch_server -): - remote_server = launch_server() - browser = await browser_type.connect(remote_server.ws_endpoint) - event_payloads = [] - browser.on("disconnected", lambda b: event_payloads.append(b)) - await browser.close() - assert event_payloads[0] == browser - - -async def test_browser_type_connect_set_browser_connected_state( - browser_type: BrowserType, launch_server -): - remote_server = launch_server() - browser = await browser_type.connect(remote_server.ws_endpoint) - assert browser.is_connected() - await browser.close() - assert browser.is_connected() is False - - -async def test_browser_type_connect_should_throw_when_used_after_is_connected_returns_false( +async def test_connect_to_closed_server_without_hangs( browser_type: BrowserType, launch_server ): remote_server = launch_server() - browser = await browser_type.connect(remote_server.ws_endpoint) - page = await browser.new_page() - remote_server.kill() - - with pytest.raises(Error) as exc_info: - await page.evaluate("1 + 1") - assert "Playwright connection closed" == exc_info.value.message - assert browser.is_connected() is False - - -async def test_browser_type_connect_should_reject_navigation_when_browser_closes( - server: Server, browser_type: BrowserType, launch_server -): - remote_server = launch_server() - browser = await browser_type.connect(remote_server.ws_endpoint) - page = await browser.new_page() - await browser.close() - - with pytest.raises(Error) as exc_info: - await page.goto(server.PREFIX + "/one-style.html") - assert "Playwright connection closed" in exc_info.value.message - - -async def test_should_not_allow_getting_the_path( - browser_type: BrowserType, launch_server, server: Server -): - def handle_download(request): - request.setHeader("Content-Type", "application/octet-stream") - request.setHeader("Content-Disposition", "attachment") - request.write(b"Hello world") - request.finish() - - server.set_route("/download", handle_download) - - remote_server = launch_server() - browser = await browser_type.connect(remote_server.ws_endpoint) - page = await browser.new_page(accept_downloads=True) - await page.set_content(f'download') - async with page.expect_download() as download_info: - await page.click("a") - download = await download_info.value with pytest.raises(Error) as exc: - await download.path() - assert ( - exc.value.message - == "Path is not available when using browser_type.connect(). Use save_as() to save a local copy." - ) - remote_server.kill() - - -async def test_prevent_getting_video_path( - browser_type: BrowserType, launch_server, tmpdir, server -): - remote_server = launch_server() - browser = await browser_type.connect(remote_server.ws_endpoint) - page = await browser.new_page(record_video_dir=tmpdir) - await page.goto(server.PREFIX + "/grid.html") - await browser.close() - assert page.video - with pytest.raises(Error) as exc: - await page.video.path() - assert ( - exc.value.message - == "Path is not available when using browserType.connect(). Use save_as() to save a local copy." - ) - remote_server.kill() + await browser_type.connect(remote_server.ws_endpoint) + assert "playwright's websocket endpoint connection error" in exc.value.message diff --git a/tests/sync/test_browsertype_connect.py b/tests/sync/test_browsertype_connect.py index d698d8da1..3fa95a879 100644 --- a/tests/sync/test_browsertype_connect.py +++ b/tests/sync/test_browsertype_connect.py @@ -145,3 +145,13 @@ def test_browser_type_connect_should_forward_close_events_to_pages( assert events == ["page::close", "context::close", "browser::disconnected"] remote.kill() assert events == ["page::close", "context::close", "browser::disconnected"] + + +def test_connect_to_closed_server_without_hangs( + browser_type: BrowserType, launch_server +): + remote_server = launch_server() + remote_server.kill() + with pytest.raises(Error) as exc: + browser_type.connect(remote_server.ws_endpoint) + assert "playwright's websocket endpoint connection error" in exc.value.message From ace076a2de14d7823b433b349292608e0b092f44 Mon Sep 17 00:00:00 2001 From: dm Date: Tue, 27 Apr 2021 10:41:13 +0800 Subject: [PATCH 07/10] revert: tests for connect --- tests/async/test_browsertype_connect.py | 125 ++++++++++++++++++++++++ 1 file changed, 125 insertions(+) diff --git a/tests/async/test_browsertype_connect.py b/tests/async/test_browsertype_connect.py index 1ffaa6fc3..55266035f 100644 --- a/tests/async/test_browsertype_connect.py +++ b/tests/async/test_browsertype_connect.py @@ -58,6 +58,131 @@ async def test_browser_type_connect_should_be_able_to_connect_two_browsers_at_th # original browser should still work assert await page2.evaluate("7 * 6") == 42 + await browser2.close() + + +async def test_browser_type_connect_disconnected_event_should_be_emitted_when_browser_is_closed_or_server_is_closed( + browser_type: BrowserType, launch_server +): + # Launch another server to not affect other tests. + remote = launch_server() + + browser1 = await browser_type.connect(remote.ws_endpoint) + browser2 = await browser_type.connect(remote.ws_endpoint) + + disconnected1 = [] + disconnected2 = [] + browser1.on("disconnected", lambda: disconnected1.append(True)) + browser2.on("disconnected", lambda: disconnected2.append(True)) + + page2 = await browser2.new_page() + + await browser1.close() + assert len(disconnected1) == 1 + assert len(disconnected2) == 0 + + remote.kill() + assert len(disconnected1) == 1 + + with pytest.raises(Error): + # Tickle connection so that it gets a chance to dispatch disconnect event. + await page2.title() + assert len(disconnected2) == 1 + + +async def test_browser_type_disconnected_event_should_have_browser_as_argument( + browser_type: BrowserType, launch_server +): + remote_server = launch_server() + browser = await browser_type.connect(remote_server.ws_endpoint) + event_payloads = [] + browser.on("disconnected", lambda b: event_payloads.append(b)) + await browser.close() + assert event_payloads[0] == browser + + +async def test_browser_type_connect_set_browser_connected_state( + browser_type: BrowserType, launch_server +): + remote_server = launch_server() + browser = await browser_type.connect(remote_server.ws_endpoint) + assert browser.is_connected() + await browser.close() + assert browser.is_connected() is False + + +async def test_browser_type_connect_should_throw_when_used_after_is_connected_returns_false( + browser_type: BrowserType, launch_server +): + remote_server = launch_server() + browser = await browser_type.connect(remote_server.ws_endpoint) + page = await browser.new_page() + + remote_server.kill() + + with pytest.raises(Error) as exc_info: + await page.evaluate("1 + 1") + assert "Playwright connection closed" == exc_info.value.message + assert browser.is_connected() is False + + +async def test_browser_type_connect_should_reject_navigation_when_browser_closes( + server: Server, browser_type: BrowserType, launch_server +): + remote_server = launch_server() + browser = await browser_type.connect(remote_server.ws_endpoint) + page = await browser.new_page() + await browser.close() + + with pytest.raises(Error) as exc_info: + await page.goto(server.PREFIX + "/one-style.html") + assert "Playwright connection closed" in exc_info.value.message + + +async def test_should_not_allow_getting_the_path( + browser_type: BrowserType, launch_server, server: Server +): + def handle_download(request): + request.setHeader("Content-Type", "application/octet-stream") + request.setHeader("Content-Disposition", "attachment") + request.write(b"Hello world") + request.finish() + + server.set_route("/download", handle_download) + + remote_server = launch_server() + browser = await browser_type.connect(remote_server.ws_endpoint) + page = await browser.new_page(accept_downloads=True) + await page.set_content(f'download') + async with page.expect_download() as download_info: + await page.click("a") + download = await download_info.value + with pytest.raises(Error) as exc: + await download.path() + assert ( + exc.value.message + == "Path is not available when using browser_type.connect(). Use save_as() to save a local copy." + ) + remote_server.kill() + + +async def test_prevent_getting_video_path( + browser_type: BrowserType, launch_server, tmpdir, server +): + remote_server = launch_server() + browser = await browser_type.connect(remote_server.ws_endpoint) + page = await browser.new_page(record_video_dir=tmpdir) + await page.goto(server.PREFIX + "/grid.html") + await browser.close() + assert page.video + with pytest.raises(Error) as exc: + await page.video.path() + assert ( + exc.value.message + == "Path is not available when using browserType.connect(). Use save_as() to save a local copy." + ) + remote_server.kill() + async def test_connect_to_closed_server_without_hangs( browser_type: BrowserType, launch_server From f0f417b82d31932ce76959d280a10357ada07c0c Mon Sep 17 00:00:00 2001 From: dm Date: Tue, 27 Apr 2021 11:39:16 +0800 Subject: [PATCH 08/10] fix(transport): test for threads --- playwright/_impl/_browser_type.py | 1 + playwright/_impl/_transport.py | 13 +++++++++---- playwright/async_api/_context_manager.py | 3 ++- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/playwright/_impl/_browser_type.py b/playwright/_impl/_browser_type.py index 613f3ff91..c5b8d207e 100644 --- a/playwright/_impl/_browser_type.py +++ b/playwright/_impl/_browser_type.py @@ -172,6 +172,7 @@ async def connect( self._connection._object_factory, transport, ) + await connection._transport.start() connection._is_sync = self._connection._is_sync connection._loop = self._connection._loop connection._loop.create_task(connection.run()) diff --git a/playwright/_impl/_transport.py b/playwright/_impl/_transport.py index 517555c4a..a53b3070a 100644 --- a/playwright/_impl/_transport.py +++ b/playwright/_impl/_transport.py @@ -42,8 +42,8 @@ def _get_stderr_fileno() -> Optional[int]: class Transport(ABC): def __init__(self) -> None: + self.on_error_future: asyncio.Future self.on_message = lambda _: None - self.on_error_future: asyncio.Future = asyncio.Future() @abstractmethod def request_stop(self) -> None: @@ -56,9 +56,14 @@ def dispose(self) -> None: async def wait_until_stopped(self) -> None: pass - async def run(self) -> None: + async def start(self) -> None: + if not hasattr(self, "on_error_future"): + self.on_error_future = asyncio.Future() self._loop = asyncio.get_running_loop() + async def run(self) -> None: + pass + @abstractmethod def send(self, message: Dict) -> None: pass @@ -93,7 +98,7 @@ async def wait_until_stopped(self) -> None: await self._proc.wait() async def run(self) -> None: - await super().run() + await self.start() self._stopped_future: asyncio.Future = asyncio.Future() try: @@ -168,7 +173,7 @@ async def wait_until_stopped(self) -> None: await self._connection.wait_closed() async def run(self) -> None: - await super().run() + await self.start() options = {} if self.timeout is not None: diff --git a/playwright/async_api/_context_manager.py b/playwright/async_api/_context_manager.py index 68a9a986c..196da2cc1 100644 --- a/playwright/async_api/_context_manager.py +++ b/playwright/async_api/_context_manager.py @@ -32,10 +32,11 @@ async def __aenter__(self) -> AsyncPlaywright: ) loop = asyncio.get_running_loop() self._connection._loop = loop - loop.create_task(self._connection.run()) obj = asyncio.ensure_future( self._connection.wait_for_object_with_known_name("Playwright") ) + await self._connection._transport.start() + loop.create_task(self._connection.run()) done, pending = await asyncio.wait( { obj, From e3fc083558f8b219a478422c9ef1e68390d9a7c7 Mon Sep 17 00:00:00 2001 From: dm Date: Tue, 27 Apr 2021 18:03:50 +0800 Subject: [PATCH 09/10] fix: change asyncio.ensure_future -> asyncio.create_task --- playwright/_impl/_browser_type.py | 2 +- playwright/async_api/_context_manager.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/playwright/_impl/_browser_type.py b/playwright/_impl/_browser_type.py index c5b8d207e..5d1539c11 100644 --- a/playwright/_impl/_browser_type.py +++ b/playwright/_impl/_browser_type.py @@ -176,7 +176,7 @@ async def connect( connection._is_sync = self._connection._is_sync connection._loop = self._connection._loop connection._loop.create_task(connection.run()) - obj = asyncio.ensure_future( + obj = asyncio.create_task( connection.wait_for_object_with_known_name("Playwright") ) done, pending = await asyncio.wait( diff --git a/playwright/async_api/_context_manager.py b/playwright/async_api/_context_manager.py index 196da2cc1..f8c7cd50b 100644 --- a/playwright/async_api/_context_manager.py +++ b/playwright/async_api/_context_manager.py @@ -32,7 +32,7 @@ async def __aenter__(self) -> AsyncPlaywright: ) loop = asyncio.get_running_loop() self._connection._loop = loop - obj = asyncio.ensure_future( + obj = asyncio.create_task( self._connection.wait_for_object_with_known_name("Playwright") ) await self._connection._transport.start() From 352d07c42f4f26a678f657d565ef9f519da2f8fc Mon Sep 17 00:00:00 2001 From: dm Date: Tue, 27 Apr 2021 18:04:30 +0800 Subject: [PATCH 10/10] fix: make run as abstractmethod --- playwright/_impl/_transport.py | 1 + 1 file changed, 1 insertion(+) diff --git a/playwright/_impl/_transport.py b/playwright/_impl/_transport.py index a53b3070a..df44bd99a 100644 --- a/playwright/_impl/_transport.py +++ b/playwright/_impl/_transport.py @@ -61,6 +61,7 @@ async def start(self) -> None: self.on_error_future = asyncio.Future() self._loop = asyncio.get_running_loop() + @abstractmethod async def run(self) -> None: pass