villebro commented on code in PR #43028:
URL: https://github.com/apache/superset/pull/43028#discussion_r3761518069
##########
superset/mcp_service/screenshot/webdriver_pool.py:
##########
@@ -16,422 +16,30 @@
# under the License.
"""
-WebDriver connection pooling for improved screenshot performance
-"""
-
-import logging
-import signal
-import threading
-import time
-from contextlib import contextmanager
-from dataclasses import dataclass
-from queue import Empty, Full, Queue
-from typing import Any, Dict, Generator
-
-from flask import current_app
-from selenium.common.exceptions import WebDriverException
-from selenium.webdriver.remote.webdriver import WebDriver
-
-from superset.utils.webdriver import WebDriverSelenium, WindowSize
-
-logger = logging.getLogger(__name__)
-
-
-class WebDriverCreationError(Exception):
- """Exception raised when WebDriver creation times out"""
-
- pass
+Stub module retained for import compatibility after Selenium removal.
-
-def _timeout_handler(signum: int, frame: Any) -> None:
- """Signal handler for WebDriver creation timeout"""
- raise WebDriverCreationError("WebDriver creation timed out")
-
-
-@dataclass
-class PooledWebDriver:
- """Wrapper for pooled WebDriver instance with metadata"""
-
- driver: WebDriver
- created_at: float
- last_used: float
- window_size: WindowSize
- user_id: int | None = None
- is_healthy: bool = True
- usage_count: int = 0
+Playwright manages its own long-lived browser instance via the
+_PlaywrightBrowserManager in superset.utils.webdriver (one browser per worker
+process, isolated contexts per screenshot). This module re-exports the pool
+accessor interface so callers that were previously using the Selenium-based
+WebDriverPool can migrate without touching import sites.
+"""
class WebDriverPool:
- """
- Connection pool for WebDriver instances to improve screenshot performance.
-
- Features:
- - Reuses WebDriver instances across requests
- - Automatic health checking and recovery
- - TTL-based expiration to prevent memory leaks
- - Thread-safe operations
- - Per-user driver isolation for security
- """
-
- def __init__(
- self,
- max_pool_size: int = 5,
- max_age_seconds: int = 3600, # 1 hour
- max_usage_count: int = 50, # Recreate after 50 uses
- idle_timeout_seconds: int = 300, # 5 minutes
- health_check_interval: int = 60, # 1 minute
- creation_timeout_seconds: int = 30, # SECURITY FIX: Timeout for
driver creation
- ):
- self.max_pool_size = max_pool_size
- self.max_age_seconds = max_age_seconds
- self.max_usage_count = max_usage_count
- self.idle_timeout_seconds = idle_timeout_seconds
- self.health_check_interval = health_check_interval
- self.creation_timeout_seconds = creation_timeout_seconds
-
- # Thread-safe pool management
- self._pool: Queue[PooledWebDriver] = Queue(maxsize=max_pool_size)
- self._active_drivers: Dict[int, PooledWebDriver] = {}
- self._lock = threading.RLock()
- self._last_health_check = time.time()
-
- # Pool statistics
- self._stats = {
- "created": 0,
- "destroyed": 0,
- "borrowed": 0,
- "returned": 0,
- "health_check_failures": 0,
- "evictions": 0,
- }
-
- def get_stats(self) -> Dict[str, Any]:
- """Get pool statistics for monitoring"""
- with self._lock:
- return {
- **self._stats,
- "pool_size": self._pool.qsize(),
- "active_count": len(self._active_drivers),
- "max_pool_size": self.max_pool_size,
- }
-
- def _create_driver(
- self, window_size: WindowSize, user_id: int | None = None
- ) -> PooledWebDriver:
- """Create a new WebDriver instance with timeout protection"""
- driver = None
- old_handler = None
-
- try:
- # SECURITY FIX: Set up timeout protection for driver creation
- old_handler = signal.signal(signal.SIGALRM, _timeout_handler)
- signal.alarm(self.creation_timeout_seconds)
-
- driver_type = current_app.config.get("WEBDRIVER_TYPE", "firefox")
- selenium_driver = WebDriverSelenium(driver_type, window_size)
-
- # Create the actual WebDriver with timeout protection
- driver = selenium_driver.create()
- driver.set_window_size(*window_size)
-
- # Clear the alarm - creation successful
- signal.alarm(0)
-
- pooled_driver = PooledWebDriver(
- driver=driver,
- created_at=time.time(),
- last_used=time.time(),
- window_size=window_size,
- user_id=user_id,
- is_healthy=True,
- usage_count=0,
- )
-
- self._stats["created"] += 1
- logger.debug(
- "Created new WebDriver instance for window size %s",
window_size
- )
- return pooled_driver
-
- except WebDriverCreationError:
- logger.error(
- "WebDriver creation timed out after %s seconds",
- self.creation_timeout_seconds,
- )
- if driver:
- try:
- driver.quit()
- except Exception:
- logger.debug("Failed to cleanup driver during timeout")
- raise Exception("WebDriver creation timed out") from None
-
- except Exception as e:
- logger.error("Failed to create WebDriver: %s", e)
- if driver:
- try:
- driver.quit()
- except Exception:
- logger.debug("Failed to cleanup driver during error")
- raise
-
- finally:
- # Restore original signal handler and clear alarm
- signal.alarm(0)
- if old_handler is not None:
- signal.signal(signal.SIGALRM, old_handler)
-
- def _is_driver_valid(self, pooled_driver: PooledWebDriver) -> bool:
- """Check if a pooled driver is still valid for use"""
- now = time.time()
-
- # Check age limit
- if now - pooled_driver.created_at > self.max_age_seconds:
- logger.debug("Driver expired due to age")
- return False
-
- # Check usage count limit
- if pooled_driver.usage_count >= self.max_usage_count:
- logger.debug("Driver expired due to usage count")
- return False
-
- # Check idle timeout
- if now - pooled_driver.last_used > self.idle_timeout_seconds:
- logger.debug("Driver expired due to idle timeout")
- return False
+ """Stub retained for import compatibility; no longer functional."""
Review Comment:
Same here - if the webdriver pool is just an empty shell after this, we
should just yank all references to it.
##########
superset/mcp_service/screenshot/webdriver_pool.py:
##########
@@ -16,422 +16,30 @@
# under the License.
"""
-WebDriver connection pooling for improved screenshot performance
-"""
-
-import logging
-import signal
-import threading
-import time
-from contextlib import contextmanager
-from dataclasses import dataclass
-from queue import Empty, Full, Queue
-from typing import Any, Dict, Generator
-
-from flask import current_app
-from selenium.common.exceptions import WebDriverException
-from selenium.webdriver.remote.webdriver import WebDriver
-
-from superset.utils.webdriver import WebDriverSelenium, WindowSize
-
-logger = logging.getLogger(__name__)
-
-
-class WebDriverCreationError(Exception):
- """Exception raised when WebDriver creation times out"""
-
- pass
+Stub module retained for import compatibility after Selenium removal.
Review Comment:
I would rather see these types of backwards compatibility stubs go, and just
update all references accordingly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
