Skip to content

Conversation

@dyxushuai
Copy link
Contributor

Context

I am creating a new reqwest::Client (builder + build) per request in two hot paths:

  • HTTP transport send_message
  • did:web resolution fetching did.json

This adds avoidable overhead and prevents effective connection reuse (pooling).

Notes

  • No public API changes; behavior should be identical except that rare certificate parsing failures now surface as errors rather than panics.

Copilot AI review requested due to automatic review settings December 14, 2025 13:58
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR optimizes HTTP client creation in the TSP SDK by implementing connection pooling through lazy static initialization using OnceCell. The changes eliminate the overhead of creating new reqwest::Client instances on every request in two critical hot paths: HTTP transport message sending and did:web resolution. Additionally, the PR improves error handling by replacing .unwrap() calls with proper map_err() conversions for certificate parsing failures.

Key changes:

  • Introduced http_client() helper functions in both tsp_sdk/src/vid/did/web.rs and tsp_sdk/src/transport/http.rs that return a static, lazily-initialized reqwest::Client
  • Converted certificate parsing from panicking on failure (.unwrap()) to returning errors (.map_err())
  • Refactored send_message() and resolve() functions to use the shared client instances

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
tsp_sdk/src/vid/did/web.rs Adds http_client() helper with OnceCell-based static client, refactors resolve() to reuse the client, and improves error handling for certificate parsing
tsp_sdk/src/transport/http.rs Adds http_client() helper with OnceCell-based static client, refactors send_message() to reuse the client, and improves error handling for certificate parsing

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@dyxushuai dyxushuai force-pushed the perf/tsp-sdk-reuse-reqwest-client branch from 54cb0f6 to 2b828cc Compare December 14, 2025 14:12
Copy link
Contributor

@wenjing wenjing left a comment

Choose a reason for hiding this comment

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

Review Comments

1. Scope Clarification

This PR optimizes HTTP client reuse for:

  • did:web resolution
  • ✅ HTTP message transport (send_message)

Note: did:webvh resolution is handled by the external didwebvh-rs crate which creates its own reqwest::Client per call — that would require a separate upstream fix.

2. Security Assessment

The once_cell crate (v1.21.3) has no known vulnerabilities. The historical issue (RUSTSEC-2019-0017) was fixed in v1.0.1 and affected Lazy<T>, not the OnceCell pattern used here. Additionally, once_cell::sync::OnceCell has been adopted into Rust's standard library as std::sync::OnceLock (Rust 1.70+), indicating thorough vetting by the Rust team.

3. Benchmark Results

Testing shows ~80% performance improvement for sequential DID resolutions:

Test Before After Improvement
1 DID 110ms 21ms ~80% faster
2 DIDs 218ms 44ms ~80% faster
4 DIDs 438ms 94ms ~78% faster

4. Additional Notes

  • Error handling is improved: removed .unwrap() panics in certificate parsing, now returns proper errors
  • The receiving path (WebSocket) is not affected by this change
  • Client-side connection pooling is consistent with standard HTTP client best practices (similar to MCP's approach)

Optional follow-up: Consider adding request timeouts to the shared client in a future PR.

Copy link
Contributor

@wenjing wenjing left a comment

Choose a reason for hiding this comment

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

Approved

This PR provides a well-implemented performance optimization with no security concerns. The code quality is good, tests pass, and the benchmark results validate the improvement.

Apply cargo fmt to fix CI rust-fmt check.

Signed-off-by: Wenjing Chu <[email protected]>
@wenjing wenjing merged commit df3ee76 into openwallet-foundation-labs:main Dec 19, 2025
12 checks passed
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