Skip to content

Update sever. (Add threading.Condition for create_task)#1144

Merged
llmc-reviewer merged 3 commits into
mainfrom
server
Jun 11, 2026
Merged

Update sever. (Add threading.Condition for create_task)#1144
llmc-reviewer merged 3 commits into
mainfrom
server

Conversation

@helloyongyang

Copy link
Copy Markdown
Contributor

No description provided.

@llmc-reviewer llmc-reviewer merged commit 93ca51a into main Jun 11, 2026
2 checks passed
@llmc-reviewer llmc-reviewer deleted the server branch June 11, 2026 09:01

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +123 to +128
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")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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)

Comment on lines 133 to +135
if not wait_task.done():
wait_task.cancel()
await asyncio.gather(wait_task, return_exceptions=True)
await asyncio.wait({wait_task}, timeout=0)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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)

Comment on lines +3 to +4
lightx2v_path=/root/yongyang3/LightX2V
test_json=/root/test.json

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants