feat(http): reuse http connection #102
Conversation
Summary of ChangesHello @kevinlin09, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant performance enhancement by implementing HTTP connection pooling for both synchronous and asynchronous operations within the SDK. By centralizing the management of HTTP client sessions, the changes aim to reduce the overhead of establishing new connections for every request, leading to lower latency and improved resource utilization. The new API provides flexible control over connection pool settings, ensuring that users can optimize network interactions according to their specific application needs, while maintaining backward compatibility for existing codebases. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 a valuable feature for reusing HTTP connections for both synchronous and asynchronous requests, which should significantly improve performance by reducing latency. The implementation is well-structured, introducing SessionManager and AioSessionManager to handle connection pooling, and is accompanied by a comprehensive set of unit tests.
The changes are well-designed, maintaining backward compatibility while allowing users to opt-in to the new connection pooling feature. The public API is clear and provides good control over the pool's behavior.
I've identified one high-severity bug in the synchronous SessionManager where calling enable() with new parameters on an already-enabled pool does not apply the new configuration. I've provided a detailed comment with a suggested fix for this issue.
Overall, this is a great contribution. Once the identified issue is addressed, this will be a solid addition to the library.
| def enable( | ||
| self, | ||
| pool_connections: Optional[int] = None, | ||
| pool_maxsize: Optional[int] = None, | ||
| max_retries: Optional[int] = None, | ||
| pool_block: Optional[bool] = None, | ||
| ): | ||
| """ | ||
| 启用连接复用 | ||
|
|
||
| Args: | ||
| pool_connections: 连接池大小，默认 10 | ||
| pool_maxsize: 最大连接数，默认 20 | ||
| max_retries: 重试次数，默认 3 | ||
| pool_block: 连接池满时是否阻塞，默认 False | ||
|
|
||
| Examples: | ||
| # 使用默认配置 | ||
| enable() | ||
|
|
||
| # 使用命名参数 | ||
| enable(pool_connections=50, pool_maxsize=100) | ||
| """ | ||
| with self._session_lock: | ||
| # 使用命名参数更新配置 | ||
| if pool_connections is not None: | ||
| self._config.pool_connections = pool_connections | ||
| if pool_maxsize is not None: | ||
| self._config.pool_maxsize = pool_maxsize | ||
| if max_retries is not None: | ||
| self._config.max_retries = max_retries | ||
| if pool_block is not None: | ||
| self._config.pool_block = pool_block | ||
|
|
||
| # 参数验证 | ||
| if self._config.pool_maxsize < self._config.pool_connections: | ||
| raise ValueError( | ||
| f"pool_maxsize ({self._config.pool_maxsize}) 必须 >= " | ||
| f"pool_connections ({self._config.pool_connections})", | ||
| ) | ||
|
|
||
| self._enabled = True | ||
| self._ensure_session() | ||
| logger.info( | ||
| "HTTP connection pool enabled with config: %s", | ||
| self._config, | ||
| ) |
There was a problem hiding this comment.
The current implementation of the enable method has a bug. If enable is called with new configuration parameters after the connection pool is already enabled, the new configuration is not applied to the active session. The session is only created once in _ensure_session if self._session is None.
This is inconsistent with the behavior of configure and the AioSessionManager.enable method, which both correctly recreate the session to apply new settings.
To fix this, the enable method should check if the configuration has changed and, if so, close the existing session so that a new one with the updated configuration can be created. This ensures that dynamically changing the pool settings via enable works as expected.
def enable(
self,
pool_connections: Optional[int] = None,
pool_maxsize: Optional[int] = None,
max_retries: Optional[int] = None,
pool_block: Optional[bool] = None,
):
"""
启用连接复用
Args:
pool_connections: 连接池大小，默认 10
pool_maxsize: 最大连接数，默认 20
max_retries: 重试次数，默认 3
pool_block: 连接池满时是否阻塞，默认 False
Examples:
# 使用默认配置
enable()
# 使用命名参数
enable(pool_connections=50, pool_maxsize=100)
"""
with self._session_lock:
config_changed = any(
p is not None
for p in [pool_connections, pool_maxsize, max_retries, pool_block]
)
if config_changed:
# 使用命名参数更新配置
if pool_connections is not None:
self._config.pool_connections = pool_connections
if pool_maxsize is not None:
self._config.pool_maxsize = pool_maxsize
if max_retries is not None:
self._config.max_retries = max_retries
if pool_block is not None:
self._config.pool_block = pool_block
# 参数验证
if self._config.pool_maxsize < self._config.pool_connections:
raise ValueError(
f"pool_maxsize ({self._config.pool_maxsize}) 必须 >= "
f"pool_connections ({self._config.pool_connections})",
)
# If already enabled and config changed, recreate session to apply new config.
if self._enabled and self._session:
try:
self._session.close()
except Exception as e:
logger.warning("Error closing session during re-enable: %s", e)
finally:
self._session = None
self._enabled = True
self._ensure_session()
logger.info(
"HTTP connection pool enabled with config: %s",
self._config,
)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]