Dev/issue#135
Conversation
- Add detailed exception logging for sync and async HTTP streaming requests - Include request context such as url, method, stream, timeout, status code, and request id - Handle SSE done events consistently across streaming parsers - Avoid swallowing stream read failures while preserving original exception behavior - Fix pylint warning by avoiding redefinition of built-in id
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several enhancements and bug fixes to the DashScope SDK. Key changes include adding support for wait_timeout_seconds in asynchronous task polling, introducing the AioTextReRank class for asynchronous text reranking, respecting the trust_env configuration in HTTP client sessions, improving SSE stream handling to properly reset event types on empty lines, and robustly resolving local file URIs. Additionally, connection lifecycle management for real-time TTS was enhanced with threading events. The code review feedback highlights two opportunities for improvement: first, ensuring that the sync version of call() in base_api.py copies kwargs to prevent losing custom configurations during polling; second, refactoring the session creation wait logic in qwen_tts_realtime.py into a polling loop to avoid timeout accumulation and unnecessary blocking when the websocket closes.
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.
| wait_kwargs = {} | ||
| if wait_timeout_seconds is not None: | ||
| wait_kwargs["wait_timeout_seconds"] = wait_timeout_seconds |
There was a problem hiding this comment.
In the sync version of call(), wait_kwargs is initialized as an empty dictionary ({}), which means any additional keyword arguments (such as headers or base_address) passed to call() are lost and not forwarded to cls.wait(). In contrast, the async version (BaseAsyncAioApi.call()) correctly copies kwargs using kwargs.copy().
To ensure consistency and prevent custom configurations from being ignored during the polling phase, wait_kwargs should be initialized as a copy of kwargs.
| wait_kwargs = {} | |
| if wait_timeout_seconds is not None: | |
| wait_kwargs["wait_timeout_seconds"] = wait_timeout_seconds | |
| wait_kwargs = kwargs.copy() | |
| if wait_timeout_seconds is not None: | |
| wait_kwargs["wait_timeout_seconds"] = wait_timeout_seconds |
| if not self.session_created_event.wait(timeout): | ||
| raise TimeoutError( | ||
| "websocket session could not be created within 5s. " | ||
| f"{self._build_connection_state_message()}", | ||
| ) |
There was a problem hiding this comment.
There are two issues with using self.session_created_event.wait(timeout) here:
- Timeout Accumulation: Since
timeout(5 seconds) is passed directly towait(), the total potential wait time can be up to 10 seconds (5s for the connection loop + 5s for the session event), which contradicts the error message stating it could not be created within 5s. - Unnecessary Blocking on Close: If the websocket connection is closed (e.g., due to a handshake/auth failure) while waiting for the session to be created,
wait()will still block for the remaining duration because it does not monitorself.websocket_closed_event.
Using a polling loop that checks both events and respects the overall start time solves both issues.
| if not self.session_created_event.wait(timeout): | |
| raise TimeoutError( | |
| "websocket session could not be created within 5s. " | |
| f"{self._build_connection_state_message()}", | |
| ) | |
| while ( | |
| not self.session_created_event.is_set() | |
| and not self.websocket_closed_event.is_set() | |
| and (time.time() - start_time) < timeout | |
| ): | |
| time.sleep(0.1) | |
| if not self.session_created_event.is_set(): | |
| raise TimeoutError( | |
| "websocket session could not be created within 5s. " | |
| f"{self._build_connection_state_message()}", | |
| ) |
1 participant
Description
[Describe what this PR does and why]
Related Issue: Fixes #[issue_number] or Relates to #[issue_number]
Security Considerations: [Check if API keys or sensitive credentials are exposed in code/logs]
Type of Change
Component(s) Affected
Checklist
Testing
[How to test these changes]
Additional Notes
[Optional: any other context]