Revert "fix: cleanup pending route handlers on close (#1412)" (#1429)

Author: rwoll

playwright/_impl/_browser_context.py

@@ -48,7 +48,6 @@
 from playwright._impl._frame import Frame
 from playwright._impl._har_router import HarRouter
 from playwright._impl._helper import (
-    BackgroundTaskTracker,
     HarRecordingMetadata,
     RouteFromHarNotFoundPolicy,
     RouteHandler,
@@ -104,7 +103,6 @@ def __init__(
         self._request: APIRequestContext = from_channel(
             initializer["APIRequestContext"]
         )
-        self._background_task_tracker: BackgroundTaskTracker = BackgroundTaskTracker()
         self._channel.on(
             "bindingCall",
             lambda params: self._on_binding(from_channel(params["binding"])),
@@ -115,7 +113,7 @@ def __init__(
         )
         self._channel.on(
             "route",
-            lambda params: self._background_task_tracker.create_task(
+            lambda params: asyncio.create_task(
                 self._on_route(
                     from_channel(params.get("route")),
                     from_channel(params.get("request")),
@@ -165,14 +163,8 @@ def __init__(
             ),
         )
         self._closed_future: asyncio.Future = asyncio.Future()
-
-        def _on_close(_: Any) -> None:
-            self._background_task_tracker.close()
-            self._closed_future.set_result(True)
-
         self.once(
-            self.Events.Close,
-            _on_close,
+            self.Events.Close, lambda context: self._closed_future.set_result(True)
         )
 
     def __repr__(self) -> str:
@@ -195,10 +187,7 @@ async def _on_route(self, route: Route, request: Request) -> None:
                 handled = await route_handler.handle(route, request)
             finally:
                 if len(self._routes) == 0:
-                    try:
-                        await self._disable_interception()
-                    except Exception:
-                        pass
+                    asyncio.create_task(self._disable_interception())
             if handled:
                 return
         await route._internal_continue(is_internal=True)

playwright/_impl/_helper.py

@@ -362,19 +362,3 @@ def is_file_payload(value: Optional[Any]) -> bool:
         and "mimeType" in value
         and "buffer" in value
     )
-
-
-class BackgroundTaskTracker:
-    def __init__(self) -> None:
-        self._pending_tasks: List[asyncio.Task] = []
-
-    def create_task(self, coro: Coroutine) -> asyncio.Task:
-        task = asyncio.create_task(coro)
-        task.add_done_callback(lambda task: self._pending_tasks.remove(task))
-        self._pending_tasks.append(task)
-        return task
-
-    def close(self) -> None:
-        for task in self._pending_tasks:
-            if not task.done():
-                task.cancel()

playwright/_impl/_network.py

@@ -223,8 +223,7 @@ def _report_handled(self, done: bool) -> None:
         chain = self._handling_future
         assert chain
         self._handling_future = None
-        if not chain.done():
-            chain.set_result(done)
+        chain.set_result(done)
 
     def _check_not_handled(self) -> None:
         if not self._handling_future:

playwright/_impl/_page.py

@@ -55,7 +55,6 @@
 from playwright._impl._frame import Frame
 from playwright._impl._har_router import HarRouter
 from playwright._impl._helper import (
-    BackgroundTaskTracker,
     ColorScheme,
     DocumentLoadState,
     ForcedColors,
@@ -152,7 +151,6 @@ def __init__(
             self._browser_context._timeout_settings
         )
         self._video: Optional[Video] = None
-        self._background_task_tracker = BackgroundTaskTracker()
         self._opener = cast("Page", from_nullable_channel(initializer.get("opener")))
 
         self._channel.on(
@@ -194,7 +192,7 @@ def __init__(
         )
         self._channel.on(
             "route",
-            lambda params: self._browser_context._background_task_tracker.create_task(
+            lambda params: asyncio.create_task(
                 self._on_route(
                     from_channel(params["route"]), from_channel(params["request"])
                 )
@@ -248,10 +246,7 @@ async def _on_route(self, route: Route, request: Request) -> None:
                 handled = await route_handler.handle(route, request)
             finally:
                 if len(self._routes) == 0:
-                    try:
-                        await self._disable_interception()
-                    except Exception:
-                        pass
+                    asyncio.create_task(self._disable_interception())
             if handled:
                 return
         await self._browser_context._on_route(route, request)

tests/async/test_browsercontext_request_intercept.py

@@ -174,20 +174,3 @@ async def test_should_give_access_to_the_intercepted_response_body(
         route.fulfill(response=response),
         eval_task,
     )
-
-
-async def test_should_cleanup_route_handlers_after_context_close(
-    context: BrowserContext, page: Page
-) -> None:
-    async def handle(r: Route):
-        pass
-
-    await context.route("**", handle)
-    try:
-        await page.goto("https://example.com", timeout=700)
-    except Exception:
-        pass
-    await context.close()
-
-    for task in asyncio.all_tasks():
-        assert "_on_route" not in str(task)

tests/async/test_har.py

@@ -503,7 +503,6 @@ async def test_should_round_trip_har_zip(
     await expect(page_2.locator("body")).to_have_css(
         "background-color", "rgb(255, 192, 203)"
     )
-    await context_2.close()
 
 
 async def test_should_round_trip_har_with_post_data(
@@ -537,7 +536,6 @@ async def test_should_round_trip_har_with_post_data(
     assert await page_2.evaluate(fetch_function, "3") == "3"
     with pytest.raises(Exception):
         await page_2.evaluate(fetch_function, "4")
-    await context_2.close()
 
 
 async def test_should_disambiguate_by_header(
@@ -580,7 +578,6 @@ async def test_should_disambiguate_by_header(
     assert await page_2.evaluate(fetch_function, "baz2") == "baz2"
     assert await page_2.evaluate(fetch_function, "baz3") == "baz3"
     assert await page_2.evaluate(fetch_function, "baz4") == "baz1"
-    await context_2.close()
 
 
 async def test_should_produce_extracted_zip(
@@ -608,7 +605,6 @@ async def test_should_produce_extracted_zip(
     await expect(page_2.locator("body")).to_have_css(
         "background-color", "rgb(255, 192, 203)"
     )
-    await context_2.close()
 
 
 async def test_should_update_har_zip_for_context(
@@ -631,7 +627,6 @@ async def test_should_update_har_zip_for_context(
     await expect(page_2.locator("body")).to_have_css(
         "background-color", "rgb(255, 192, 203)"
     )
-    await context_2.close()
 
 
 async def test_should_update_har_zip_for_page(
@@ -654,7 +649,6 @@ async def test_should_update_har_zip_for_page(
     await expect(page_2.locator("body")).to_have_css(
         "background-color", "rgb(255, 192, 203)"
     )
-    await context_2.close()
 
 
 async def test_should_update_extracted_har_zip_for_page(
@@ -681,4 +675,3 @@ async def test_should_update_extracted_har_zip_for_page(
     await expect(page_2.locator("body")).to_have_css(
         "background-color", "rgb(255, 192, 203)"
     )
-    await context_2.close()

tests/async/test_request_intercept.py

@@ -17,7 +17,7 @@
 
 from twisted.web import http
 
-from playwright.async_api import BrowserContext, Page, Route
+from playwright.async_api import Page, Route
 from tests.server import Server
 
 
@@ -168,20 +168,3 @@ async def test_should_give_access_to_the_intercepted_response_body(
         route.fulfill(response=response),
         eval_task,
     )
-
-
-async def test_should_cleanup_route_handlers_after_context_close(
-    context: BrowserContext, page: Page
-) -> None:
-    async def handle(r: Route):
-        pass
-
-    await page.route("**", handle)
-    try:
-        await page.goto("https://example.com", timeout=700)
-    except Exception:
-        pass
-    await context.close()
-
-    for task in asyncio.all_tasks():
-        assert "_on_route" not in str(task)

tests/sync/test_browsercontext_request_intercept.py

@@ -12,7 +12,6 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-import asyncio
 from pathlib import Path
 
 from twisted.web import http
@@ -122,17 +121,3 @@ def handle_route(route: Route) -> None:
     assert request.uri.decode() == "/title.html"
     original = (assetdir / "title.html").read_text()
     assert response.text() == original
-
-
-def test_should_cleanup_route_handlers_after_context_close(
-    context: BrowserContext, page: Page
-) -> None:
-    context.route("**", lambda r: None)
-    try:
-        page.goto("https://example.com", timeout=700)
-    except Exception:
-        pass
-    context.close()
-
-    for task in asyncio.all_tasks():
-        assert "_on_route" not in str(task)

tests/sync/test_har.py

@@ -471,7 +471,6 @@ def test_should_round_trip_har_with_post_data(
     assert page_2.evaluate(fetch_function, "3") == "3"
     with pytest.raises(Exception):
         page_2.evaluate(fetch_function, "4")
-    context_2.close()
 
 
 def test_should_disambiguate_by_header(
@@ -513,7 +512,6 @@ def test_should_disambiguate_by_header(
     assert page_2.evaluate(fetch_function, "baz2") == "baz2"
     assert page_2.evaluate(fetch_function, "baz3") == "baz3"
     assert page_2.evaluate(fetch_function, "baz4") == "baz1"
-    context_2.close()
 
 
 def test_should_produce_extracted_zip(
@@ -539,7 +537,6 @@ def test_should_produce_extracted_zip(
     page_2.goto(server.PREFIX + "/one-style.html")
     assert "hello, world!" in page_2.content()
     expect(page_2.locator("body")).to_have_css("background-color", "rgb(255, 192, 203)")
-    context_2.close()
 
 
 def test_should_update_har_zip_for_context(
@@ -560,7 +557,6 @@ def test_should_update_har_zip_for_context(
     page_2.goto(server.PREFIX + "/one-style.html")
     assert "hello, world!" in page_2.content()
     expect(page_2.locator("body")).to_have_css("background-color", "rgb(255, 192, 203)")
-    context_2.close()
 
 
 def test_should_update_har_zip_for_page(
@@ -581,7 +577,6 @@ def test_should_update_har_zip_for_page(
     page_2.goto(server.PREFIX + "/one-style.html")
     assert "hello, world!" in page_2.content()
     expect(page_2.locator("body")).to_have_css("background-color", "rgb(255, 192, 203)")
-    context_2.close()
 
 
 def test_should_update_extracted_har_zip_for_page(
@@ -606,4 +601,3 @@ def test_should_update_extracted_har_zip_for_page(
     page_2.goto(server.PREFIX + "/one-style.html")
     assert "hello, world!" in page_2.content()
     expect(page_2.locator("body")).to_have_css("background-color", "rgb(255, 192, 203)")
-    context_2.close()

tests/sync/test_request_intercept.py

@@ -12,12 +12,11 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-import asyncio
 from pathlib import Path
 
 from twisted.web import http
 
-from playwright.sync_api import BrowserContext, Page, Route
+from playwright.sync_api import Page, Route
 from tests.server import Server
 
 
@@ -116,17 +115,3 @@ def handle_route(route: Route) -> None:
     assert request.uri.decode() == "/title.html"
     original = (assetdir / "title.html").read_text()
     assert response.text() == original
-
-
-def test_should_cleanup_route_handlers_after_context_close(
-    context: BrowserContext, page: Page
-) -> None:
-    page.route("**", lambda r: None)
-    try:
-        page.goto("https://example.com", timeout=700)
-    except Exception:
-        pass
-    context.close()
-
-    for task in asyncio.all_tasks():
-        assert "_on_route" not in str(task)