Update sever. (Add threading.Condition for create_task)#1144
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces performance monitoring, latency logging, and a more efficient task-polling mechanism using a threading.Condition variable instead of sleep-based polling. It also updates the runner's return format to support multiple images and adds test scripts for OpenAI-compatible image generation. The review feedback highlights potential task leaks in the asynchronous cancellation logic within openai_images.py and recommends replacing asyncio.wait(..., timeout=0) with asyncio.gather(..., return_exceptions=True). Additionally, it is recommended to remove hardcoded absolute paths in the newly added shell script to ensure portability.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| for pending_task in pending: | ||
| pending_task.cancel() | ||
| if pending: | ||
| _, still_pending = await asyncio.wait(pending, timeout=0) | ||
| if still_pending: | ||
| logger.debug(f"Task {task_id} disconnect watcher cancellation is still pending") |
There was a problem hiding this comment.
Using asyncio.wait(pending, timeout=0) does not actually await the cancellation of the pending tasks, which can lead to task leaks and "Task was destroyed but it is pending!" warnings in the event loop. Since _watch_client_disconnect is a simple loop that yields control via asyncio.sleep, awaiting its cancellation is extremely fast and safe.
It is highly recommended to use the established pattern of await asyncio.gather(*pending, return_exceptions=True) to ensure proper cleanup of cancelled tasks.
| for pending_task in pending: | |
| pending_task.cancel() | |
| if pending: | |
| _, still_pending = await asyncio.wait(pending, timeout=0) | |
| if still_pending: | |
| logger.debug(f"Task {task_id} disconnect watcher cancellation is still pending") | |
| for pending_task in pending: | |
| pending_task.cancel() | |
| if pending: | |
| await asyncio.gather(*pending, return_exceptions=True) |
| if not wait_task.done(): | ||
| wait_task.cancel() | ||
| await asyncio.gather(wait_task, return_exceptions=True) | ||
| await asyncio.wait({wait_task}, timeout=0) |
There was a problem hiding this comment.
Similarly, using asyncio.wait({wait_task}, timeout=0) does not await the cancellation of wait_task, which can leave the task pending in the event loop and trigger warnings. Please use await asyncio.gather(wait_task, return_exceptions=True) to properly await the cancelled task.
| if not wait_task.done(): | |
| wait_task.cancel() | |
| await asyncio.gather(wait_task, return_exceptions=True) | |
| await asyncio.wait({wait_task}, timeout=0) | |
| if not wait_task.done(): | |
| wait_task.cancel() | |
| await asyncio.gather(wait_task, return_exceptions=True) |
| lightx2v_path=/root/yongyang3/LightX2V | ||
| test_json=/root/test.json |
There was a problem hiding this comment.
The script hardcodes absolute paths specific to a local environment (/root/yongyang3/LightX2V and /root/test.json). This makes the script non-portable and likely to fail on other machines or for other users.
Consider resolving the paths dynamically relative to the script's directory, or using environment variables with sensible defaults.
| lightx2v_path=/root/yongyang3/LightX2V | |
| test_json=/root/test.json | |
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | |
| lightx2v_path="$(realpath "${SCRIPT_DIR}/../..")" | |
| test_json="${lightx2v_path}/test.json" |
No description provided.