-
-
Notifications
You must be signed in to change notification settings - Fork 5.9k
fix: prevent memory leak by closing unused context #1640
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
da82f0a
a234959
c85f56b
a046203
5196b95
43b197a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -611,12 +611,16 @@ def __init__(self, browser_config: BrowserConfig, logger=None, use_undetected: b | |||||||||||||||||||||||||||||||
| # Keep track of contexts by a "config signature," so each unique config reuses a single context | ||||||||||||||||||||||||||||||||
| self.contexts_by_config = {} | ||||||||||||||||||||||||||||||||
| self._contexts_lock = asyncio.Lock() | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| # Reference counting for contexts - tracks how many requests are using each context | ||||||||||||||||||||||||||||||||
| # Key: config_signature, Value: count of active requests using this context | ||||||||||||||||||||||||||||||||
| self._context_refcounts = {} | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| # Serialize context.new_page() across concurrent tasks to avoid races | ||||||||||||||||||||||||||||||||
| # when using a shared persistent context (context.pages may be empty | ||||||||||||||||||||||||||||||||
| # for all racers). Prevents 'Target page/context closed' errors. | ||||||||||||||||||||||||||||||||
| self._page_lock = asyncio.Lock() | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| # Stealth adapter for stealth mode | ||||||||||||||||||||||||||||||||
| self._stealth_adapter = None | ||||||||||||||||||||||||||||||||
| if self.config.enable_stealth and not self.use_undetected: | ||||||||||||||||||||||||||||||||
|
|
@@ -1102,6 +1106,9 @@ async def get_page(self, crawlerRunConfig: CrawlerRunConfig): | |||||||||||||||||||||||||||||||
| await self.setup_context(context, crawlerRunConfig) | ||||||||||||||||||||||||||||||||
| self.contexts_by_config[config_signature] = context | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| # Increment reference count - this context is now in use | ||||||||||||||||||||||||||||||||
| self._context_refcounts[config_signature] = self._context_refcounts.get(config_signature, 0) + 1 | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| # Create a new page from the chosen context | ||||||||||||||||||||||||||||||||
| page = await context.new_page() | ||||||||||||||||||||||||||||||||
| await self._apply_stealth_to_page(page) | ||||||||||||||||||||||||||||||||
|
|
@@ -1137,11 +1144,127 @@ def _cleanup_expired_sessions(self): | |||||||||||||||||||||||||||||||
| for sid in expired_sessions: | ||||||||||||||||||||||||||||||||
| asyncio.create_task(self.kill_session(sid)) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| async def cleanup_contexts(self, max_contexts: int = 5, force: bool = False): | ||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||
| Clean up contexts to prevent memory growth. | ||||||||||||||||||||||||||||||||
| Only closes contexts that have no active references AND no open pages (safe cleanup). | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| Args: | ||||||||||||||||||||||||||||||||
| max_contexts: Maximum number of contexts to keep. Excess idle contexts | ||||||||||||||||||||||||||||||||
| will be closed, starting with the oldest ones. | ||||||||||||||||||||||||||||||||
| force: If True, close contexts even if they have pages (but never if refcount > 0). | ||||||||||||||||||||||||||||||||
| Use with caution. | ||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||
| async with self._contexts_lock: | ||||||||||||||||||||||||||||||||
| # First, identify contexts that are safe to close: | ||||||||||||||||||||||||||||||||
| # - No active references (refcount == 0) | ||||||||||||||||||||||||||||||||
| # - No open pages (or force=True) | ||||||||||||||||||||||||||||||||
| idle_contexts = [] | ||||||||||||||||||||||||||||||||
| active_contexts = [] | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| for sig, ctx in list(self.contexts_by_config.items()): | ||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||
| refcount = self._context_refcounts.get(sig, 0) | ||||||||||||||||||||||||||||||||
| has_pages = hasattr(ctx, 'pages') and len(ctx.pages) > 0 | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| # Context is safe to close only if refcount is 0 | ||||||||||||||||||||||||||||||||
| if refcount > 0: | ||||||||||||||||||||||||||||||||
| # Context is actively being used by a request - never close | ||||||||||||||||||||||||||||||||
| active_contexts.append((sig, ctx)) | ||||||||||||||||||||||||||||||||
| elif has_pages and not force: | ||||||||||||||||||||||||||||||||
| # Has pages but no refs - might be finishing up, skip unless forced | ||||||||||||||||||||||||||||||||
| active_contexts.append((sig, ctx)) | ||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||
| # refcount == 0 and (no pages or force=True) - safe to close | ||||||||||||||||||||||||||||||||
| idle_contexts.append((sig, ctx)) | ||||||||||||||||||||||||||||||||
| except Exception: | ||||||||||||||||||||||||||||||||
| # Context may be in bad state, only cleanup if no refs | ||||||||||||||||||||||||||||||||
| if self._context_refcounts.get(sig, 0) == 0: | ||||||||||||||||||||||||||||||||
| idle_contexts.append((sig, ctx)) | ||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||
| active_contexts.append((sig, ctx)) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| # Log context status for debugging | ||||||||||||||||||||||||||||||||
| self.logger.debug( | ||||||||||||||||||||||||||||||||
| message="Context cleanup check: {total} total, {idle} idle (refcount=0), {active} active", | ||||||||||||||||||||||||||||||||
| tag="CLEANUP", | ||||||||||||||||||||||||||||||||
| params={ | ||||||||||||||||||||||||||||||||
| "total": len(self.contexts_by_config), | ||||||||||||||||||||||||||||||||
| "idle": len(idle_contexts), | ||||||||||||||||||||||||||||||||
| "active": len(active_contexts) | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| # Close idle contexts if we exceed max_contexts total | ||||||||||||||||||||||||||||||||
| contexts_to_close = [] | ||||||||||||||||||||||||||||||||
| if len(self.contexts_by_config) > max_contexts: | ||||||||||||||||||||||||||||||||
| # Calculate how many we need to close | ||||||||||||||||||||||||||||||||
| excess = len(self.contexts_by_config) - max_contexts | ||||||||||||||||||||||||||||||||
| # Only close from idle contexts (safe) | ||||||||||||||||||||||||||||||||
| contexts_to_close = idle_contexts[:excess] | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| # If force=True and we still have too many, close active ones too | ||||||||||||||||||||||||||||||||
| if force and len(self.contexts_by_config) - len(contexts_to_close) > max_contexts: | ||||||||||||||||||||||||||||||||
| remaining_excess = len(self.contexts_by_config) - len(contexts_to_close) - max_contexts | ||||||||||||||||||||||||||||||||
| contexts_to_close.extend(active_contexts[:remaining_excess]) | ||||||||||||||||||||||||||||||||
|
Comment on lines
+1206
to
+1209
|
||||||||||||||||||||||||||||||||
| # If force=True and we still have too many, close active ones too | |
| if force and len(self.contexts_by_config) - len(contexts_to_close) > max_contexts: | |
| remaining_excess = len(self.contexts_by_config) - len(contexts_to_close) - max_contexts | |
| contexts_to_close.extend(active_contexts[:remaining_excess]) | |
| # If force=True and we still have too many, close additional contexts | |
| # but never close contexts with refcount > 0 (they may be in active use). | |
| if force and len(self.contexts_by_config) - len(contexts_to_close) > max_contexts: | |
| remaining_excess = len(self.contexts_by_config) - len(contexts_to_close) - max_contexts | |
| # From active_contexts, only consider those whose refcount is 0 for forced closure | |
| force_closable_active = [ | |
| (sig, ctx) | |
| for sig, ctx in active_contexts | |
| if self._context_refcounts.get(sig, 0) == 0 | |
| ] | |
| contexts_to_close.extend(force_closable_active[:remaining_excess]) |
Copilot
AI
Jan 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| pass | |
| except Exception as e: | |
| # Ignore individual page close failures but record them for diagnostics | |
| self.logger.warning( | |
| message="Error closing page during context cleanup: {error}", | |
| tag="WARNING", | |
| params={"error": str(e)} | |
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The release_context call here creates a reference counting imbalance when using session_id. Looking at browser_manager.py get_page(), when a session_id is provided and already exists, the function returns early (line 1063-1066) without incrementing the refcount. However, this release_context will still be called, decrementing a counter that was never incremented. This will cause the refcount to go negative (though clamped to 0 by the max() call in release_context), potentially allowing contexts to be cleaned up while still in use by sessions. The condition should also check that no session_id is being used, similar to: if not self.browser_config.use_managed_browser and not config.session_id: