fix: hanging context managers (microsoft#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: