diff --git a/src/crawlee/browsers/_browser_controller.py b/src/crawlee/browsers/_browser_controller.py index 8a69d8fe38..0555b142d9 100644 --- a/src/crawlee/browsers/_browser_controller.py +++ b/src/crawlee/browsers/_browser_controller.py @@ -54,6 +54,11 @@ def idle_time(self) -> timedelta: def has_free_capacity(self) -> bool: """Return if the browser has free capacity to open a new page.""" + @property + def is_opening_pages(self) -> bool: + """Return if the browser has any `new_page` calls currently in flight.""" + return False + @property @abstractmethod def is_browser_connected(self) -> bool: diff --git a/src/crawlee/browsers/_browser_pool.py b/src/crawlee/browsers/_browser_pool.py index 5d6005c978..a4c549d35d 100644 --- a/src/crawlee/browsers/_browser_pool.py +++ b/src/crawlee/browsers/_browser_pool.py @@ -381,14 +381,14 @@ async def _launch_new_browser(self, page_id: str, plugin: BrowserPlugin) -> Brow def _identify_inactive_browsers(self) -> None: """Identify inactive browsers and move them to the inactive list if their idle time exceeds the threshold.""" for browser in list(self._active_browsers): - if browser.idle_time >= self._browser_inactive_threshold: + if browser.idle_time >= self._browser_inactive_threshold and not browser.is_opening_pages: self._active_browsers.remove(browser) self._inactive_browsers.append(browser) async def _close_inactive_browsers(self) -> None: """Close the browsers that have no active pages and have been idle for a certain period.""" for browser in list(self._inactive_browsers): - if not browser.pages: + if not browser.pages and not browser.is_opening_pages: await browser.close() self._inactive_browsers.remove(browser) diff --git a/src/crawlee/browsers/_playwright_browser_controller.py b/src/crawlee/browsers/_playwright_browser_controller.py index c773c9eefe..ae40fee43e 100644 --- a/src/crawlee/browsers/_playwright_browser_controller.py +++ b/src/crawlee/browsers/_playwright_browser_controller.py @@ -139,6 +139,11 @@ def idle_time(self) -> timedelta: def has_free_capacity(self) -> bool: return (self.pages_count + self._opening_pages_count) < self._max_open_pages_per_browser + @property + @override + def is_opening_pages(self) -> bool: + return self._opening_pages_count > 0 + @property @override def is_browser_connected(self) -> bool: diff --git a/src/crawlee/browsers/_stagehand_browser_controller.py b/src/crawlee/browsers/_stagehand_browser_controller.py index 9263a30ebb..ff60327775 100644 --- a/src/crawlee/browsers/_stagehand_browser_controller.py +++ b/src/crawlee/browsers/_stagehand_browser_controller.py @@ -105,6 +105,11 @@ def idle_time(self) -> timedelta: def has_free_capacity(self) -> bool: return (self.pages_count + self._opening_pages_count) < self._max_open_pages_per_browser + @property + @override + def is_opening_pages(self) -> bool: + return self._opening_pages_count > 0 + @property @override def is_browser_connected(self) -> bool: diff --git a/tests/unit/browsers/test_browser_pool.py b/tests/unit/browsers/test_browser_pool.py index 2d101d3475..0aba86b599 100644 --- a/tests/unit/browsers/test_browser_pool.py +++ b/tests/unit/browsers/test_browser_pool.py @@ -1,12 +1,13 @@ from __future__ import annotations +import asyncio from datetime import timedelta from typing import TYPE_CHECKING from unittest.mock import AsyncMock import pytest -from crawlee.browsers import BrowserPool, PlaywrightBrowserPlugin +from crawlee.browsers import BrowserPool, PlaywrightBrowserController, PlaywrightBrowserPlugin from crawlee.browsers._browser_controller import BrowserController from crawlee.browsers._types import CrawleePage @@ -14,6 +15,7 @@ from collections.abc import Mapping from typing import Any + from playwright.async_api import BrowserContext, Page from yarl import URL from crawlee.browsers._browser_plugin import BrowserPlugin @@ -159,6 +161,51 @@ async def test_resource_management(server_url: URL) -> None: assert page.page.is_closed() +async def test_reaper_does_not_close_browser_with_page_opening_in_flight(monkeypatch: pytest.MonkeyPatch) -> None: + """The inactive-browser reaper leaves a browser alone while its `new_page` call is still in flight.""" + opening_in_flight = asyncio.Event() + resume_opening = asyncio.Event() + original_create_context = PlaywrightBrowserController._create_browser_context + + async def create_context_with_gated_first_page( + self: PlaywrightBrowserController, *args: Any, **kwargs: Any + ) -> BrowserContext: + context = await original_create_context(self, *args, **kwargs) + original_new_page = context.new_page + + async def gated_new_page(*new_page_args: Any, **new_page_kwargs: Any) -> Page: + opening_in_flight.set() + await resume_opening.wait() + return await original_new_page(*new_page_args, **new_page_kwargs) + + monkeypatch.setattr(context, 'new_page', gated_new_page) + return context + + monkeypatch.setattr(PlaywrightBrowserController, '_create_browser_context', create_context_with_gated_first_page) + + # Long reaper intervals so that only the manual calls below drive the reaping; a zero inactivity + # threshold makes the freshly launched browser eligible for it right away. + async with BrowserPool( + browser_inactive_threshold=timedelta(seconds=0), + identify_inactive_browsers_interval=timedelta(hours=1), + close_inactive_browsers_interval=timedelta(hours=1), + ) as browser_pool: + new_page_task = asyncio.create_task(browser_pool.new_page()) + await asyncio.wait_for(opening_in_flight.wait(), timeout=60) + + # Run one reaper cycle, exactly as the recurring tasks would, while the page opening is pending. + browser_pool._identify_inactive_browsers() + await browser_pool._close_inactive_browsers() + + resume_opening.set() + page = await new_page_task + + assert not page.page.is_closed() + assert browser_pool.total_pages_count == 1 + + await page.page.close() + + async def test_methods_raise_error_when_not_active() -> None: plugin = PlaywrightBrowserPlugin() browser_pool = BrowserPool([plugin])