-
Notifications
You must be signed in to change notification settings - Fork 177
Fix BrowserGym Playwright threading issues and increase WebSocket message limit #354
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: main
Are you sure you want to change the base?
Conversation
Greptile OverviewGreptile SummaryThis PR fixes critical threading issues with BrowserGym's Playwright integration and increases WebSocket message limits to handle large observations. Key Changes:
The threading fix is particularly important - it moves environment instantiation and cleanup into the executor thread, which is required for libraries like Playwright that use greenlets and thread-local storage. Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client as EnvClient
participant HTTPServer as HTTPEnvServer
participant Executor as ThreadPoolExecutor
participant Env as BrowserGym Environment
participant Playwright as Playwright/Greenlet
Note over Client,Playwright: WebSocket Session Creation
Client->>HTTPServer: WebSocket connect /ws
HTTPServer->>Executor: Create ThreadPoolExecutor(max_workers=1)
Note over HTTPServer,Executor: Store executor before env creation
HTTPServer->>Executor: run_in_executor(env_factory)
Executor->>Env: Create environment instance
Env->>Playwright: Initialize Playwright browser
Note over Playwright: All Playwright objects created in executor thread
Playwright-->>Env: Browser ready
Env-->>Executor: Environment ready
Executor-->>HTTPServer: Environment instance
HTTPServer->>HTTPServer: Store session (env + executor)
HTTPServer-->>Client: WebSocket connected
Note over Client,Playwright: Step Operations
Client->>HTTPServer: step(action) via WebSocket
HTTPServer->>Executor: run_in_executor(env.step)
Note over Executor: Same thread as env creation
Executor->>Env: step(action)
Env->>Playwright: Browser interactions
Playwright-->>Env: Observation (screenshot, DOM)
Note over Env: Large message (up to 100MB)
Env-->>Executor: Observation
Executor-->>HTTPServer: Result
HTTPServer-->>Client: Observation via WebSocket
Note over Client: max_message_size=100MB handles large obs
Note over Client,Playwright: Session Cleanup
Client->>HTTPServer: WebSocket disconnect
HTTPServer->>Executor: run_in_executor(env.close)
Note over Executor: Close in same thread as creation
Executor->>Env: close()
Env->>Playwright: Cleanup browser resources
Playwright-->>Env: Closed
Env-->>Executor: Done
Executor-->>HTTPServer: Cleanup complete
HTTPServer->>Executor: shutdown(wait=False)
HTTPServer-->>Client: Session destroyed
|
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.
4 files reviewed, 1 comment
| try: | ||
| env = self._env_factory() | ||
| # Create environment in the executor thread | ||
| loop = asyncio.get_event_loop() | ||
| env = await loop.run_in_executor(executor, self._env_factory) |
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.
environment creation inside _session_lock blocks other concurrent session operations
If _env_factory() takes significant time (e.g., launching Playwright browser), this will block other WebSocket connections from being created/destroyed. Consider releasing the lock before awaiting the executor, then re-acquiring it to update _sessions.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/openenv/core/env_server/http_server.py
Line: 297:300
Comment:
environment creation inside `_session_lock` blocks other concurrent session operations
If `_env_factory()` takes significant time (e.g., launching Playwright browser), this will block other WebSocket connections from being created/destroyed. Consider releasing the lock before awaiting the executor, then re-acquiring it to update `_sessions`.
How can I resolve this? If you propose a fix, please make it concise.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.
@sergiopaniego do we need to fix this?
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.
updated!
Summary
This PR fixes BrowserGym compatibility issues that caused exceptions when using Playwright, and increases the WebSocket message size limit to support large observations.
It's related to #343
Type of Change
Test Plan
The updated env is deployed here
Claude Code Review
"N/A"