perf: cache reqwest::Client globally for TCP connection reuse#1303
Open
lennney wants to merge 2 commits into
Open
perf: cache reqwest::Client globally for TCP connection reuse#1303lennney wants to merge 2 commits into
lennney wants to merge 2 commits into
Conversation
lennney
force-pushed
the
perf/reqwest-cache
branch
2 times, most recently
from
a042c44 to
cdac980
Compare
- OnceLock caching for TCP/TLS connection reuse across all upstream requests - User-Agent is set per-request via .header(), not baked into the cached client - Fixes UA first-caller-wins bug where dynamic UAs were silently ignored - Restore exact UA assertions in protocol_proxy tests (configured vs passthrough) - Update 12 call sites across model_catalog, stepwise, protocol_proxy, relay_config, update, plugin_marketplace, and ads
lennney
force-pushed
the
perf/reqwest-cache
branch
from
cdac980 to
d52cf73
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
该 PR 旨在通过全局缓存 reqwest::Client（使用 OnceLock 惰性初始化）来复用连接池，从而减少每次请求重新建连/TLS 握手带来的额外开销；同时将 User-Agent 从“客户端默认 header”迁移为“每个请求显式设置”。
Changes:
- 将
http_client::proxied_client改为全局缓存的OnceLock<reqwest::Client>，避免重复创建连接池。 - 将多处调用从
proxied_client(user_agent)调整为proxied_client()+ 每个请求.header("User-Agent", ...)。 - 调整 model catalog 读取路径与相关测试，新增/传递
user_agent参数以便请求侧设置 UA。
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/codex-plus-core/src/http_client.rs | 引入全局 OnceLock 缓存 reqwest::Client，不再在 client 上设置默认 UA |
| crates/codex-plus-core/src/protocol_proxy.rs | 上游请求改为复用全局 client，并在请求级别设置 UA |
| crates/codex-plus-core/src/model_catalog.rs | read_codex_model_catalog_from_home / fetch_models_from_source 增加 UA 透传并在请求侧设置 |
| crates/codex-plus-core/src/update.rs | 更新检查/下载请求改为复用全局 client，并显式设置 UA |
| crates/codex-plus-core/src/plugin_marketplace.rs | marketplace zip 下载请求复用全局 client，并显式设置 UA |
| crates/codex-plus-core/src/stepwise.rs | stepwise 请求复用全局 client，并显式设置 UA |
| crates/codex-plus-core/src/relay_config.rs | relay 测试请求复用全局 client，并显式设置 UA |
| crates/codex-plus-core/src/ads.rs | ads 拉取请求复用全局 client，并显式设置 UA |
| crates/codex-plus-core/tests/model_catalog.rs | 跟随函数签名变化，为 read_codex_model_catalog_from_home 增加 user_agent 实参 |
| crates/codex-plus-core/tests/protocol_proxy.rs | 为 UA 行为相关测试增加/调整注释 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
533
to
535
| let ua_value = effective_user_agent(&relay.user_agent, original_user_agent); | ||
| let client = crate::http_client::proxied_client()?; | ||
| let upstream = match send_upstream_request_for_responses( |
Comment on lines
651
to
653
| let ua_value = effective_user_agent(&relay.user_agent, original_user_agent); | ||
| let client = crate::http_client::proxied_client()?; | ||
| let upstream = send_upstream_request( |
Comment on lines
+698
to
+704
| let ua_value = effective_user_agent(&relay.user_agent, original_user_agent); | ||
| let client = crate::http_client::proxied_client()?; | ||
| let upstream = client | ||
| .post(chat_completions_url(&relay.base_url)) | ||
| .bearer_auth(relay.api_key.trim()) | ||
| .header(reqwest::header::CONTENT_TYPE, "application/json") | ||
| .header("User-Agent", &ua_value) |
Comment on lines
+469
to
+472
| let mut request = client | ||
| .get(&endpoint) | ||
| .header(reqwest::header::ACCEPT, "application/json"); | ||
| .header(reqwest::header::ACCEPT, "application/json") | ||
| .header("User-Agent", user_agent); |
Comment on lines
524
to
528
| .header(reqwest::header::CONTENT_TYPE, "application/json") | ||
| .header("User-Agent", "CodexPlusPlus/RelayTest") | ||
| .json(&payload) | ||
| .send() | ||
| .await?; |
Comment on lines
+1493
to
+1494
| /// Verify the proxied client sets the default CodexPlusPlus user-agent. | ||
| /// |
…eqwest default Copilot review identified 6 call sites where effective_user_agent() returning an empty string would set an empty User-Agent header, which is worse than not setting one (reqwest would use its default). Fixes: - protocol_proxy.rs: 3 call sites — guard with !ua_value.is_empty() - model_catalog.rs: 1 call site — same guard - relay_config.rs: 1 call site — add missing UA to /v1 retry request - tests/protocol_proxy.rs: update outdated comment
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Cache
reqwest::Clientin aOnceLockso the first call initialises it and subsequent calls reuse the same connection pool via cheapArcclone.Problem:
proxied_client()created a newreqwest::Clienton every call — fresh connection pool, extra TLS handshake latency per request.Fix: Global
OnceLock<reqwest::Client>— TCP/TLS connections are now reused across upstream API requests. Client is built outside the closure so build failures returnErrinstead of panicking.Behavior Change
Previously, requests without a configured user-agent would pass through the original Codex client UA. Now they always use the
CodexPlusPlus/X.Y.ZUA. Tests updated to match on prefix.Compatibility
CodexPlusPlus/*UAs)cdp.rs) keeps its own builder withno_proxy— unaffectedVerification