fix: hanging context managers (#2144)

Author: mxschmitt

playwright/_impl/_async_base.py

@@ -33,6 +33,9 @@ def __init__(self, future: "asyncio.Future[T]") -> None:
     async def value(self) -> T:
         return mapping.from_maybe_impl(await self._future)
 
+    def _cancel(self) -> None:
+        self._future.cancel()
+
     def is_done(self) -> bool:
         return self._future.done()
 
@@ -50,7 +53,10 @@ async def __aexit__(
         exc_val: BaseException,
         exc_tb: TracebackType,
     ) -> None:
-        await self._event.value
+        if exc_val:
+            self._event._cancel()
+        else:
+            await self._event.value
 
 
 class AsyncBase(ImplWrapper):

playwright/_impl/_sync_base.py

@@ -59,6 +59,9 @@ def value(self) -> T:
             raise exception
         return cast(T, mapping.from_maybe_impl(self._future.result()))
 
+    def _cancel(self) -> None:
+        self._future.cancel()
+
     def is_done(self) -> bool:
         return self._future.done()
 
@@ -76,7 +79,10 @@ def __exit__(
         exc_val: BaseException,
         exc_tb: TracebackType,
     ) -> None:
-        self._event.value
+        if exc_val:
+            self._event._cancel()
+        else:
+            self._event.value
 
 
 class SyncBase(ImplWrapper):

tests/async/test_context_manager.py

@@ -12,7 +12,9 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-from playwright.async_api import BrowserType
+import pytest
+
+from playwright.async_api import BrowserContext, BrowserType
 
 
 async def test_context_managers(browser_type: BrowserType, launch_arguments):
@@ -24,3 +26,9 @@ async def test_context_managers(browser_type: BrowserType, launch_arguments):
             assert len(browser.contexts) == 1
         assert len(browser.contexts) == 0
     assert not browser.is_connected()
+
+
+async def test_context_managers_not_hang(context: BrowserContext):
+    with pytest.raises(Exception, match="Oops!"):
+        async with await context.new_page():
+            raise Exception("Oops!")

tests/async/test_page.py

@@ -344,6 +344,14 @@ async def test_wait_for_response_should_use_context_timeout(
     assert "Timeout 1000ms exceeded" in exc_info.value.message
 
 
+async def test_expect_response_should_not_hang_when_predicate_throws(
+    page: Page,
+) -> None:
+    with pytest.raises(Exception, match="Oops!"):
+        async with page.expect_response("**/*"):
+            raise Exception("Oops!")
+
+
 async def test_expose_binding(page):
     binding_source = []
 

tests/sync/test_context_manager.py

@@ -14,7 +14,9 @@
 
 from typing import Dict
 
-from playwright.sync_api import BrowserType
+import pytest
+
+from playwright.sync_api import BrowserContext, BrowserType
 
 
 def test_context_managers(browser_type: BrowserType, launch_arguments: Dict) -> None:
@@ -26,3 +28,9 @@ def test_context_managers(browser_type: BrowserType, launch_arguments: Dict) ->
             assert len(browser.contexts) == 1
         assert len(browser.contexts) == 0
     assert not browser.is_connected()
+
+
+def test_context_managers_not_hang(context: BrowserContext) -> None:
+    with pytest.raises(Exception, match="Oops!"):
+        with context.new_page():
+            raise Exception("Oops!")

tests/sync/test_sync.py

@@ -150,10 +150,14 @@ def test_sync_wait_for_event(page: Page, server: Server) -> None:
 
 
 def test_sync_wait_for_event_raise(page: Page) -> None:
-    with pytest.raises(Error):
-        with page.expect_event("popup", timeout=500) as popup:
+    with pytest.raises(AssertionError):
+        with page.expect_event("popup", timeout=500):
             assert False
-        assert popup.value is None
+
+    with pytest.raises(Error) as exc_info:
+        with page.expect_event("popup", timeout=500):
+            page.wait_for_timeout(1_000)
+    assert "Timeout 500ms exceeded" in exc_info.value.message
 
 
 def test_sync_make_existing_page_sync(page: Page) -> None:
@@ -281,6 +285,12 @@ def test_expect_response_should_work(page: Page, server: Server) -> None:
     assert resp.value.request
 
 
+def test_expect_response_should_not_hang_when_predicate_throws(page: Page) -> None:
+    with pytest.raises(Exception, match="Oops!"):
+        with page.expect_response("**/*"):
+            raise Exception("Oops!")
+
+
 def test_expect_response_should_use_context_timeout(
     page: Page, context: BrowserContext, server: Server
 ) -> None: