From ee2a6fb77fdd834766cf72152d3cb76b7aa82273 Mon Sep 17 00:00:00 2001 From: dgtlmoon Date: Tue, 3 Feb 2026 10:51:44 +0100 Subject: [PATCH] Puppeteer and Playwright browser close/shutdown improvements --- .../content_fetchers/playwright.py | 68 ++++++++--------- .../content_fetchers/puppeteer.py | 74 ++++++++++++++----- changedetectionio/worker.py | 28 ++----- 3 files changed, 95 insertions(+), 75 deletions(-) diff --git a/changedetectionio/content_fetchers/playwright.py b/changedetectionio/content_fetchers/playwright.py index 1ea90a813..aff5c9d2d 100644 --- a/changedetectionio/content_fetchers/playwright.py +++ b/changedetectionio/content_fetchers/playwright.py @@ -1,3 +1,4 @@ +import asyncio import gc import json import os @@ -349,12 +350,7 @@ class fetcher(Fetcher): if self.status_code != 200 and not ignore_status_codes: screenshot = await capture_full_page_async(self.page, screenshot_format=self.screenshot_format, watch_uuid=watch_uuid, lock_viewport_elements=self.lock_viewport_elements) - # Cleanup before raising to prevent memory leak - await self.page.close() - await context.close() - await browser.close() - # Force garbage collection to release Playwright resources immediately - gc.collect() + # Finally block will handle cleanup raise Non200ErrorCodeReceived(url=url, status_code=self.status_code, screenshot=screenshot) if not empty_pages_are_a_change and len((await self.page.content()).strip()) == 0: @@ -370,12 +366,7 @@ class fetcher(Fetcher): try: await self.iterate_browser_steps(start_url=url) except BrowserStepsStepException: - try: - await context.close() - await browser.close() - except Exception as e: - # Fine, could be messy situation - pass + # Finally block will handle cleanup raise await self.page.wait_for_timeout(extra_wait * 1000) @@ -424,35 +415,40 @@ class fetcher(Fetcher): raise ScreenshotUnavailable(url=url, status_code=self.status_code) finally: - # Request garbage collection one more time before closing + # Clean up resources properly with timeouts to prevent hanging try: - await self.page.request_gc() - except: - pass - - # Clean up resources properly - try: - await self.page.request_gc() - except: - pass + if hasattr(self, 'page') and self.page: + await self.page.request_gc() + await asyncio.wait_for(self.page.close(), timeout=5.0) + logger.debug(f"Successfully closed page for {url}") + except asyncio.TimeoutError: + logger.warning(f"Timed out closing page for {url} (5s)") + except Exception as e: + logger.warning(f"Error closing page for {url}: {e}") + finally: + self.page = None try: - await self.page.close() - except: - pass - self.page = None + if context: + await asyncio.wait_for(context.close(), timeout=5.0) + logger.debug(f"Successfully closed context for {url}") + except asyncio.TimeoutError: + logger.warning(f"Timed out closing context for {url} (5s)") + except Exception as e: + logger.warning(f"Error closing context for {url}: {e}") + finally: + context = None try: - await context.close() - except: - pass - context = None - - try: - await browser.close() - except: - pass - browser = None + if browser: + await asyncio.wait_for(browser.close(), timeout=5.0) + logger.debug(f"Successfully closed browser connection for {url}") + except asyncio.TimeoutError: + logger.warning(f"Timed out closing browser connection for {url} (5s)") + except Exception as e: + logger.warning(f"Error closing browser for {url}: {e}") + finally: + browser = None # Force Python GC to release Playwright resources immediately # Playwright objects can have circular references that delay cleanup diff --git a/changedetectionio/content_fetchers/puppeteer.py b/changedetectionio/content_fetchers/puppeteer.py index e2e9e9410..f5714e812 100644 --- a/changedetectionio/content_fetchers/puppeteer.py +++ b/changedetectionio/content_fetchers/puppeteer.py @@ -1,4 +1,5 @@ import asyncio +import gc import json import os import websockets.exceptions @@ -221,19 +222,36 @@ class fetcher(Fetcher): self.browser_connection_url += f"{r}--proxy-server={proxy_url}" async def quit(self, watch=None): - try: - await self.page.close() - del self.page - except Exception as e: - pass + watch_uuid = watch.get('uuid') if watch else 'unknown' + # Close page try: - await self.browser.close() - del self.browser + if hasattr(self, 'page') and self.page: + await asyncio.wait_for(self.page.close(), timeout=5.0) + logger.debug(f"[{watch_uuid}] Page closed successfully") + except asyncio.TimeoutError: + logger.warning(f"[{watch_uuid}] Timed out closing page (5s)") except Exception as e: - pass + logger.warning(f"[{watch_uuid}] Error closing page: {e}") + finally: + self.page = None - logger.info("Cleanup puppeteer complete.") + # Close browser connection + try: + if hasattr(self, 'browser') and self.browser: + await asyncio.wait_for(self.browser.close(), timeout=5.0) + logger.debug(f"[{watch_uuid}] Browser closed successfully") + except asyncio.TimeoutError: + logger.warning(f"[{watch_uuid}] Timed out closing browser (5s)") + except Exception as e: + logger.warning(f"[{watch_uuid}] Error closing browser: {e}") + finally: + self.browser = None + + logger.info(f"[{watch_uuid}] Cleanup puppeteer complete") + + # Force garbage collection to release resources + gc.collect() async def fetch_page(self, current_include_filters, @@ -263,9 +281,11 @@ class fetcher(Fetcher): # Connect directly using the specified browser_ws_endpoint # @todo timeout try: + logger.debug(f"[{watch_uuid}] Connecting to browser at {self.browser_connection_url}") self.browser = await pyppeteer_instance.connect(browserWSEndpoint=self.browser_connection_url, ignoreHTTPSErrors=True ) + logger.debug(f"[{watch_uuid}] Browser connected successfully") except websockets.exceptions.InvalidStatusCode as e: raise BrowserConnectError(msg=f"Error while trying to connect the browser, Code {e.status_code} (check your access, whitelist IP, password etc)") except websockets.exceptions.InvalidURI: @@ -274,7 +294,18 @@ class fetcher(Fetcher): raise BrowserConnectError(msg=f"Error connecting to the browser - Exception '{str(e)}'") # more reliable is to just request a new page - self.page = await self.browser.newPage() + try: + logger.debug(f"[{watch_uuid}] Creating new page") + self.page = await self.browser.newPage() + logger.debug(f"[{watch_uuid}] Page created successfully") + except Exception as e: + logger.error(f"[{watch_uuid}] Failed to create new page: {e}") + # Browser is connected but page creation failed - must cleanup browser + try: + await asyncio.wait_for(self.browser.close(), timeout=3.0) + except Exception as cleanup_error: + logger.error(f"[{watch_uuid}] Failed to cleanup browser after page creation failure: {cleanup_error}") + raise # Add console handler to capture console.log from favicon fetcher #self.page.on('console', lambda msg: logger.debug(f"Browser console [{msg.type}]: {msg.text}")) @@ -343,6 +374,12 @@ class fetcher(Fetcher): w = extra_wait - 2 if extra_wait > 4 else 2 logger.debug(f"Waiting {w} seconds before calling Page.stopLoading...") await asyncio.sleep(w) + + # Check if page still exists (might have been closed due to error during sleep) + if not self.page or not hasattr(self.page, '_client'): + logger.debug("Page already closed, skipping stopLoading") + return + logger.debug("Issuing stopLoading command...") await self.page._client.send('Page.stopLoading') logger.debug("stopLoading command sent!") @@ -368,7 +405,9 @@ class fetcher(Fetcher): asyncio.create_task(handle_frame_navigation()) response = await self.page.goto(url, timeout=0) await asyncio.sleep(1 + extra_wait) - await self.page._client.send('Page.stopLoading') + # Check if page still exists before sending command + if self.page and hasattr(self.page, '_client'): + await self.page._client.send('Page.stopLoading') if response: break @@ -437,15 +476,9 @@ class fetcher(Fetcher): logger.debug(f"Screenshot format {self.screenshot_format}") self.screenshot = await capture_full_page(page=self.page, screenshot_format=self.screenshot_format, watch_uuid=watch_uuid, lock_viewport_elements=self.lock_viewport_elements) - # Force aggressive memory cleanup - pyppeteer base64 decode creates temporary buffers + # Force garbage collection - pyppeteer base64 decode creates temporary buffers import gc gc.collect() - # Release C-level memory from base64 decode back to OS - try: - import ctypes - ctypes.CDLL('libc.so.6').malloc_trim(0) - except Exception: - pass self.xpath_data = await self.page.evaluate(XPATH_ELEMENT_JS, { "visualselector_xpath_selectors": visualselector_xpath_selectors, "max_height": MAX_TOTAL_HEIGHT @@ -499,6 +532,11 @@ class fetcher(Fetcher): ) except asyncio.TimeoutError: raise (BrowserFetchTimedOut(msg=f"Browser connected but was unable to process the page in {max_time} seconds.")) + finally: + # CRITICAL: Always cleanup browser connections regardless of success/failure + # This ensures connections are closed even when exceptions occur + # Reuse quit() method to avoid code duplication + await self.quit(watch={'uuid': watch_uuid} if watch_uuid else None) # Plugin registration for built-in fetcher diff --git a/changedetectionio/worker.py b/changedetectionio/worker.py index e5046c5ef..3dfb8acf3 100644 --- a/changedetectionio/worker.py +++ b/changedetectionio/worker.py @@ -475,14 +475,9 @@ async def async_update_worker(worker_id, q, notification_q, app, datastore, exec del update_handler update_handler = None - # Force aggressive memory cleanup after clearing + # Force garbage collection import gc gc.collect() - try: - import ctypes - ctypes.CDLL('libc.so.6').malloc_trim(0) - except Exception: - pass except Exception as e: logger.error(f"Worker {worker_id} unexpected error processing {uuid}: {e}") @@ -495,6 +490,7 @@ async def async_update_worker(worker_id, q, notification_q, app, datastore, exec finally: # Always cleanup - this runs whether there was an exception or not if uuid: + # Call quit() as backup (Puppeteer/Playwright have internal cleanup, but this acts as safety net) try: if update_handler and hasattr(update_handler, 'fetcher') and update_handler.fetcher: await update_handler.fetcher.quit(watch=watch) @@ -503,35 +499,25 @@ async def async_update_worker(worker_id, q, notification_q, app, datastore, exec try: # Release UUID from processing (thread-safe) worker_pool.release_uuid_from_processing(uuid, worker_id=worker_id) - + # Send completion signal if watch: - #logger.info(f"Worker {worker_id} sending completion signal for UUID {watch['uuid']}") watch_check_update.send(watch_uuid=watch['uuid']) - # Explicitly clean up update_handler and all its references + # Clean up all memory references BEFORE garbage collection if update_handler: - # Clear fetcher content using the proper method if hasattr(update_handler, 'fetcher') and update_handler.fetcher: update_handler.fetcher.clear_content() - - # Clear processor references if hasattr(update_handler, 'content_processor'): update_handler.content_processor = None - + del update_handler update_handler = None - # Clear local contents variable if it still exists + # Clear large content variables if 'contents' in locals(): del contents - # Note: We don't set watch = None here because: - # 1. watch is just a local reference to datastore.data['watching'][uuid] - # 2. Setting it to None doesn't affect the datastore - # 3. GC can't collect the object anyway (still referenced by datastore) - # 4. It would just cause confusion - - # Force garbage collection after cleanup + # Force garbage collection after all references are cleared import gc gc.collect()