feat: share single HTTP client across all registry clients#55
Conversation
Implements connection reuse by sharing a single reqwest::Client across all 8 registry implementations. This enables HTTP/2 multiplexing, reduces TLS handshake overhead, and lowers memory usage from separate connection pools. Changes: - Add http_client.rs with create_shared_client() function - Update all registries to accept Arc<Client> via with_client() constructor - Backend creates one shared client and passes it to all registries - Keep Default impl for backward compatibility Closes #31
|
Warning Rate limit exceeded@mpiton has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 8 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughReplaced per-registry reqwest::Client instances with a shared, Arc-wrapped Client via a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Backend as DependiBackend
participant Factory as SharedClientFactory
participant CI as CratesIoRegistry
participant NP as NpmRegistry
participant PY as PyPiRegistry
participant GP as GoProxyRegistry
participant PK as PackagistRegistry
participant PD as PubDevRegistry
participant NU as NuGetRegistry
participant RB as RubyGemsRegistry
participant Ext as ExternalRegistryAPIs
rect rgb(220,240,255)
Note over Backend,Factory: Initialization — create and distribute shared client
Backend->>Factory: create_shared_client()
Factory-->>Backend: Arc<Client>
Backend->>CI: CratesIoRegistry::with_client(Arc::clone(&client))
Backend->>NP: NpmRegistry::with_client(Arc::clone(&client))
Backend->>PY: PyPiRegistry::with_client(Arc::clone(&client))
Backend->>GP: GoProxyRegistry::with_client(Arc::clone(&client))
Backend->>PK: PackagistRegistry::with_client(Arc::clone(&client))
Backend->>PD: PubDevRegistry::with_client(Arc::clone(&client))
Backend->>NU: NuGetRegistry::with_client(Arc::clone(&client))
Backend->>RB: RubyGemsRegistry::with_client(Arc::clone(&client))
end
rect rgb(235,255,235)
Note over CI,Ext: Runtime request flow using shared client
CI->>Ext: HTTP request via shared Arc<Client>
Ext-->>CI: HTTP response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dependi-lsp/src/registries/http_client.rs (1)
28-39: LGTM! Proper shared client creation.The function correctly builds and wraps the
reqwest::Clientin anArcfor thread-safe sharing. The configuration enables the key benefits outlined in the PR: HTTP/2 multiplexing, connection reuse, and reduced TLS overhead.💡 Optional: Consider adding integration tests
While the unit tests verify Arc behavior, you could add integration tests that verify multiple registry instances actually share the same underlying client. For example:
#[test] fn test_registries_share_client() { let shared = create_shared_client().expect("Failed to create client"); let registry1 = CratesIoRegistry::with_client(Arc::clone(&shared)); let registry2 = NpmRegistry::with_client(Arc::clone(&shared)); // Verify both registries use the same client instance assert!(Arc::strong_count(&shared) == 3); }This could help document the intended usage pattern and catch any future regressions.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
dependi-lsp/src/backend.rsdependi-lsp/src/bin/test_dates.rsdependi-lsp/src/registries/crates_io.rsdependi-lsp/src/registries/go_proxy.rsdependi-lsp/src/registries/http_client.rsdependi-lsp/src/registries/mod.rsdependi-lsp/src/registries/npm.rsdependi-lsp/src/registries/nuget.rsdependi-lsp/src/registries/packagist.rsdependi-lsp/src/registries/pub_dev.rsdependi-lsp/src/registries/pypi.rsdependi-lsp/src/registries/rubygems.rs
🧰 Additional context used
🧬 Code graph analysis (9)
dependi-lsp/src/registries/http_client.rs (2)
dependi-lsp/src/backend.rs (2)
client(439-440)new(107-134)dependi-lsp/src/registries/crates_io.rs (1)
new(22-27)
dependi-lsp/src/registries/rubygems.rs (3)
dependi-lsp/src/registries/http_client.rs (1)
create_shared_client(28-39)dependi-lsp/src/registries/crates_io.rs (2)
with_client(46-52)default(56-58)dependi-lsp/src/registries/pub_dev.rs (2)
with_client(20-25)default(29-31)
dependi-lsp/src/registries/packagist.rs (3)
dependi-lsp/src/registries/http_client.rs (1)
create_shared_client(28-39)dependi-lsp/src/registries/pub_dev.rs (2)
with_client(20-25)default(29-31)dependi-lsp/src/registries/pypi.rs (2)
with_client(20-25)default(29-31)
dependi-lsp/src/registries/pub_dev.rs (8)
dependi-lsp/src/registries/http_client.rs (1)
create_shared_client(28-39)dependi-lsp/src/registries/crates_io.rs (2)
with_client(46-52)default(56-58)dependi-lsp/src/registries/go_proxy.rs (2)
with_client(20-25)default(29-31)dependi-lsp/src/registries/npm.rs (2)
with_client(20-25)default(29-31)dependi-lsp/src/registries/nuget.rs (2)
with_client(20-25)default(29-31)dependi-lsp/src/registries/packagist.rs (2)
with_client(20-25)default(29-31)dependi-lsp/src/registries/pypi.rs (2)
with_client(20-25)default(29-31)dependi-lsp/src/registries/rubygems.rs (2)
with_client(23-28)default(32-34)
dependi-lsp/src/registries/pypi.rs (7)
dependi-lsp/src/registries/http_client.rs (1)
create_shared_client(28-39)dependi-lsp/src/registries/crates_io.rs (2)
with_client(46-52)default(56-58)dependi-lsp/src/registries/go_proxy.rs (2)
with_client(20-25)default(29-31)dependi-lsp/src/registries/npm.rs (2)
with_client(20-25)default(29-31)dependi-lsp/src/registries/packagist.rs (2)
with_client(20-25)default(29-31)dependi-lsp/src/registries/pub_dev.rs (2)
with_client(20-25)default(29-31)dependi-lsp/src/registries/rubygems.rs (2)
with_client(23-28)default(32-34)
dependi-lsp/src/registries/crates_io.rs (9)
dependi-lsp/src/registries/http_client.rs (1)
create_shared_client(28-39)dependi-lsp/src/registries/go_proxy.rs (2)
with_client(20-25)default(29-31)dependi-lsp/src/registries/npm.rs (2)
with_client(20-25)default(29-31)dependi-lsp/src/registries/nuget.rs (2)
with_client(20-25)default(29-31)dependi-lsp/src/registries/packagist.rs (2)
with_client(20-25)default(29-31)dependi-lsp/src/registries/pub_dev.rs (2)
with_client(20-25)default(29-31)dependi-lsp/src/registries/pypi.rs (2)
with_client(20-25)default(29-31)dependi-lsp/src/registries/rubygems.rs (2)
with_client(23-28)default(32-34)dependi-lsp/src/backend.rs (2)
client(439-440)new(107-134)
dependi-lsp/src/registries/nuget.rs (8)
dependi-lsp/src/registries/http_client.rs (1)
create_shared_client(28-39)dependi-lsp/src/registries/crates_io.rs (2)
with_client(46-52)default(56-58)dependi-lsp/src/registries/go_proxy.rs (2)
with_client(20-25)default(29-31)dependi-lsp/src/registries/npm.rs (2)
with_client(20-25)default(29-31)dependi-lsp/src/registries/packagist.rs (2)
with_client(20-25)default(29-31)dependi-lsp/src/registries/pub_dev.rs (2)
with_client(20-25)default(29-31)dependi-lsp/src/registries/pypi.rs (2)
with_client(20-25)default(29-31)dependi-lsp/src/registries/rubygems.rs (2)
with_client(23-28)default(32-34)
dependi-lsp/src/bin/test_dates.rs (8)
dependi-lsp/src/registries/crates_io.rs (1)
default(56-58)dependi-lsp/src/registries/go_proxy.rs (1)
default(29-31)dependi-lsp/src/registries/npm.rs (1)
default(29-31)dependi-lsp/src/registries/nuget.rs (1)
default(29-31)dependi-lsp/src/registries/packagist.rs (1)
default(29-31)dependi-lsp/src/registries/pub_dev.rs (1)
default(29-31)dependi-lsp/src/registries/pypi.rs (1)
default(29-31)dependi-lsp/src/registries/rubygems.rs (1)
default(32-34)
dependi-lsp/src/backend.rs (9)
dependi-lsp/src/registries/http_client.rs (1)
create_shared_client(28-39)dependi-lsp/src/registries/crates_io.rs (2)
new(22-27)with_client(46-52)dependi-lsp/src/registries/go_proxy.rs (1)
with_client(20-25)dependi-lsp/src/registries/npm.rs (1)
with_client(20-25)dependi-lsp/src/registries/nuget.rs (1)
with_client(20-25)dependi-lsp/src/registries/packagist.rs (1)
with_client(20-25)dependi-lsp/src/registries/pub_dev.rs (1)
with_client(20-25)dependi-lsp/src/registries/pypi.rs (1)
with_client(20-25)dependi-lsp/src/registries/rubygems.rs (1)
with_client(23-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (rust)
🔇 Additional comments (18)
dependi-lsp/src/bin/test_dates.rs (1)
9-9: LGTM! Clean migration to the shared client pattern.The switch from
.new().unwrap()to.default()correctly leverages the new shared HTTP client implementation. These registries now benefit from connection reuse and reduced resource overhead.Also applies to: 26-26, 43-43
dependi-lsp/src/registries/mod.rs (1)
142-142: LGTM! Proper module exposure.The new
http_clientmodule is correctly exposed, making the shared client functionality available throughout the registry system.dependi-lsp/src/registries/http_client.rs (2)
18-26: LGTM! Well-configured client defaults.The timeout and connection pooling settings are sensible:
- 10s default timeout handles most registry operations
- 5s connect timeout prevents hanging on unreachable hosts
- 90s pool idle timeout balances connection reuse with resource cleanup
- TCP keepalive helps maintain HTTP/2 connections
45-59: LGTM! Tests verify Arc semantics correctly.The tests properly verify that the client can be created and that Arc reference counting works as expected.
dependi-lsp/src/registries/rubygems.rs (2)
23-28: LGTM! Consistent with shared client pattern.The
with_clientconstructor correctly accepts and stores theArc<Client>, enabling RubyGemsRegistry to participate in connection sharing with other registries.
31-34: LGTM! Proper Default implementation.The Default trait correctly delegates to
with_clientusing the shared client. The.expect()call is appropriate here since a Default implementation should provide a working instance or fail fast.dependi-lsp/src/registries/crates_io.rs (2)
40-40: LGTM! Proper shared client integration with rate limiting.The registry correctly stores
Arc<Client>and maintains per-instance rate limiting, which is essential for respecting crates.io's API limits. The rate limiter won't interfere with connection sharing since it only controls request timing, not the underlying HTTP connections.Also applies to: 46-52
55-59: LGTM! Consistent Default implementation.The Default trait correctly delegates to
with_clientusing the shared client, enabling this registry to benefit from connection reuse while maintaining its specific rate-limiting behavior.dependi-lsp/src/registries/go_proxy.rs (1)
4-4: LGTM! Clean dependency injection pattern for shared HTTP client.The refactoring successfully enables connection reuse by:
- Storing
Arc<Client>for cheap cloning and shared ownership- Adding
with_client()constructor for explicit client injection- Maintaining backward compatibility via
Default(which creates a new client)The shared client pattern is achieved when the backend passes the same
Arc<Client>instance to multiple registries viawith_client().Also applies to: 10-10, 15-15, 20-25, 30-30
dependi-lsp/src/registries/npm.rs (1)
4-4: LGTM! Consistent implementation of shared HTTP client pattern.The changes mirror the pattern across other registries, enabling connection reuse through dependency injection. The
Defaultimplementation provides backward compatibility whilewith_client()enables the backend to share a single client instance across all registries.Also applies to: 10-10, 15-15, 20-25, 30-30
dependi-lsp/src/registries/pypi.rs (1)
4-4: LGTM! Well-aligned with project-wide HTTP client refactoring.The implementation follows the established pattern consistently. The use of
Arc<Client>enables efficient connection sharing when the same client instance is passed to multiple registries via thewith_client()constructor.Also applies to: 10-10, 15-15, 20-25, 30-30
dependi-lsp/src/registries/packagist.rs (1)
4-4: LGTM! Consistent refactoring to shared HTTP client pattern.The changes successfully implement dependency injection for the HTTP client, matching the pattern across all other registries. This enables HTTP/2 multiplexing and connection pooling when a shared
Arc<Client>is passed from the backend.Also applies to: 10-10, 15-15, 20-25, 30-30
dependi-lsp/src/registries/pub_dev.rs (1)
10-10: LGTM! Successfully aligned with shared HTTP client pattern.The changes integrate PubDevRegistry into the project-wide shared client pattern by:
- Adding the
with_client()constructor for dependency injection- Updating
Defaultto usecreate_shared_client()for consistencyThis completes the alignment with other registries, enabling connection reuse across the system.
Also applies to: 20-25, 30-30
dependi-lsp/src/registries/nuget.rs (2)
10-10: LGTM! Consistent constructor pattern across registries.The import and
with_clientconstructor correctly follow the shared HTTP client pattern used by all other registries. The dependency injection approach enables connection reuse and centralized client configuration.Also applies to: 20-25
28-31: LGTM! Default implementation maintains backward compatibility.The Default implementation correctly uses the shared client creation pattern. The
expect()call will panic if client creation fails, which is acceptable behavior at initialization time—if the HTTP client cannot be created, the LSP server cannot function. This pattern is consistent across all registry implementations.dependi-lsp/src/backend.rs (3)
26-26: LGTM! Import correctly references the shared client module.
108-108: Excellent! Single shared HTTP client enables connection reuse.The shared HTTP client creation correctly implements the PR objective of connection pooling and HTTP/2 multiplexing across all registries. The
expect()call is appropriate here—if client creation fails at initialization, the LSP server cannot function.
123-130: Excellent implementation of shared client pattern!The registry initializations correctly share a single HTTP client across all 8 registries. The Arc cloning strategy is optimal—cloning for the first seven registries and consuming the Arc for the final one (rubygems). This achieves the PR objectives of:
- Enabling HTTP/2 multiplexing across all registry requests
- Reducing TLS handshake overhead through connection reuse
- Lowering memory usage by replacing 8 connection pools with one shared pool
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @mpiton. * #55 (comment) The following files were modified: * `dependi-lsp/src/backend.rs` * `dependi-lsp/src/bin/test_dates.rs` * `dependi-lsp/src/registries/crates_io.rs` * `dependi-lsp/src/registries/go_proxy.rs` * `dependi-lsp/src/registries/http_client.rs` * `dependi-lsp/src/registries/npm.rs` * `dependi-lsp/src/registries/nuget.rs` * `dependi-lsp/src/registries/packagist.rs` * `dependi-lsp/src/registries/pub_dev.rs` * `dependi-lsp/src/registries/pypi.rs` * `dependi-lsp/src/registries/rubygems.rs`
Docstrings generation was requested by @mpiton. * #55 (comment) The following files were modified: * `dependi-lsp/src/backend.rs` * `dependi-lsp/src/bin/test_dates.rs` * `dependi-lsp/src/registries/crates_io.rs` * `dependi-lsp/src/registries/go_proxy.rs` * `dependi-lsp/src/registries/http_client.rs` * `dependi-lsp/src/registries/npm.rs` * `dependi-lsp/src/registries/nuget.rs` * `dependi-lsp/src/registries/packagist.rs` * `dependi-lsp/src/registries/pub_dev.rs` * `dependi-lsp/src/registries/pypi.rs` * `dependi-lsp/src/registries/rubygems.rs` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
dependi-lsp/src/registries/npm.rs (1)
19-38: Consider usingcreate_shared_client()in docstring example for consistency.The docstring example uses
Arc::new(Client::new())which creates a client without the configured timeouts, user-agent, and connection pooling settings. Other registries likecrates_io.rsandpypi.rsusecreate_shared_client()in their examples.📝 Suggested docstring update
/// # Examples /// /// ``` /// use std::sync::Arc; - /// // assume Client and NpmRegistry are in scope - /// let client = Arc::new(Client::new()); - /// let registry = NpmRegistry::with_client(client.clone()); + /// // assume create_shared_client() -> Arc<reqwest::Client> is available + /// let client = create_shared_client().expect("client"); + /// let registry = NpmRegistry::with_client(client); /// ```dependi-lsp/src/registries/packagist.rs (1)
19-38: Consider aligning docstring example with other registries.Similar to
npm.rs, the example usesArc::new(Client::new())rather thancreate_shared_client(). For consistency across the codebase, consider updating to matchpypi.rsandcrates_io.rs.dependi-lsp/src/registries/nuget.rs (1)
19-35: Docstring example path may not compile in doc tests.The example references
crate::registries::nuget::NuGetRegistrywhich assumes being run from within the crate. Also, similar to other registries, consider usingcreate_shared_client()for consistency.dependi-lsp/src/registries/rubygems.rs (1)
22-44: Good docstring with proper imports shown.The example correctly shows the
usestatements needed. For consistency withcrates_io.rsandpypi.rs, consider usingcreate_shared_client()instead ofArc::new(Client::new())to demonstrate the recommended pattern.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
dependi-lsp/src/backend.rsdependi-lsp/src/bin/test_dates.rsdependi-lsp/src/registries/crates_io.rsdependi-lsp/src/registries/go_proxy.rsdependi-lsp/src/registries/http_client.rsdependi-lsp/src/registries/npm.rsdependi-lsp/src/registries/nuget.rsdependi-lsp/src/registries/packagist.rsdependi-lsp/src/registries/pub_dev.rsdependi-lsp/src/registries/pypi.rsdependi-lsp/src/registries/rubygems.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- dependi-lsp/src/bin/test_dates.rs
- dependi-lsp/src/registries/pub_dev.rs
- dependi-lsp/src/registries/http_client.rs
- dependi-lsp/src/backend.rs
- dependi-lsp/src/registries/go_proxy.rs
🧰 Additional context used
🧬 Code graph analysis (5)
dependi-lsp/src/registries/packagist.rs (4)
dependi-lsp/src/registries/http_client.rs (1)
create_shared_client(46-57)dependi-lsp/src/registries/npm.rs (1)
with_client(33-38)dependi-lsp/src/registries/pub_dev.rs (1)
with_client(31-36)dependi-lsp/src/registries/pypi.rs (1)
with_client(36-41)
dependi-lsp/src/registries/nuget.rs (6)
dependi-lsp/src/registries/http_client.rs (1)
create_shared_client(46-57)dependi-lsp/src/registries/npm.rs (2)
with_client(33-38)default(49-51)dependi-lsp/src/registries/packagist.rs (2)
with_client(33-38)default(49-51)dependi-lsp/src/registries/pub_dev.rs (2)
with_client(31-36)default(47-49)dependi-lsp/src/registries/pypi.rs (2)
with_client(36-41)default(52-54)dependi-lsp/src/registries/rubygems.rs (2)
with_client(38-43)default(56-58)
dependi-lsp/src/registries/npm.rs (3)
dependi-lsp/src/registries/http_client.rs (1)
create_shared_client(46-57)dependi-lsp/src/registries/pub_dev.rs (1)
with_client(31-36)dependi-lsp/src/registries/pypi.rs (1)
with_client(36-41)
dependi-lsp/src/registries/pypi.rs (5)
dependi-lsp/src/registries/http_client.rs (1)
create_shared_client(46-57)dependi-lsp/src/registries/npm.rs (2)
with_client(33-38)default(49-51)dependi-lsp/src/registries/nuget.rs (2)
with_client(30-35)default(48-50)dependi-lsp/src/registries/pub_dev.rs (2)
with_client(31-36)default(47-49)dependi-lsp/src/registries/rubygems.rs (2)
with_client(38-43)default(56-58)
dependi-lsp/src/registries/rubygems.rs (2)
dependi-lsp/src/registries/http_client.rs (1)
create_shared_client(46-57)dependi-lsp/src/registries/pub_dev.rs (1)
with_client(31-36)
🪛 GitHub Actions: CI
dependi-lsp/src/registries/packagist.rs
[error] 1-1: cargo fmt --check failed. Formatting differences detected; run 'cargo fmt' to format the code.
dependi-lsp/src/registries/nuget.rs
[error] 1-1: cargo fmt --check failed. Formatting differences detected; run 'cargo fmt' to format the code.
dependi-lsp/src/registries/npm.rs
[error] 1-1: cargo fmt --check failed. Formatting differences detected; run 'cargo fmt' to format the code.
dependi-lsp/src/registries/pypi.rs
[error] 1-1: cargo fmt --check failed. Formatting differences detected; run 'cargo fmt' to format the code.
dependi-lsp/src/registries/crates_io.rs
[error] 1-1: cargo fmt --check failed. Formatting differences detected; run 'cargo fmt' to format the code.
dependi-lsp/src/registries/rubygems.rs
[error] 1-1: cargo fmt --check failed. Formatting differences detected; run 'cargo fmt' to format the code.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (rust)
🔇 Additional comments (8)
dependi-lsp/src/registries/crates_io.rs (2)
46-66: Implementation looks good.The
with_clientconstructor correctly accepts anArc<Client>and preserves the rate limiter initialization. The docstring example correctly shows the usage pattern withcreate_shared_client().
69-86: Implementation is correct and properly formatted; no changes needed.The
Defaultimplementation correctly delegates towith_client()and properly documents the panic condition when HTTP client creation fails. Manual inspection of the code reveals no formatting issues—line endings are correct, indentation is consistent, and there is no trailing whitespace.The implementation is sound and ready as-is.
dependi-lsp/src/registries/npm.rs (1)
41-51: Implementation is correct; runcargo fmt.The
Defaultimplementation correctly usescreate_shared_client(). Address the formatting issue reported by CI.dependi-lsp/src/registries/pypi.rs (2)
19-41: Well-documentedwith_clientconstructor.The docstring correctly demonstrates usage with
create_shared_client()and includes proper parameter documentation. This is consistent with the intended pattern.
44-54: Default implementation is correct; address formatting.Run
cargo fmtto resolve the CI pipeline failure.dependi-lsp/src/registries/packagist.rs (1)
41-51: Implementation is correct; runcargo fmt.dependi-lsp/src/registries/nuget.rs (1)
38-50: Implementation is correct; address CI formatting failure.dependi-lsp/src/registries/rubygems.rs (1)
46-58: Implementation is correct; runcargo fmt.The
Defaultimplementation follows the established pattern. Address the formatting issue flagged by CI to pass the pipeline.
Additional changes to complete shared HTTP client implementation: - Add http_client() method to Registry trait for client inspection - Implement http_client() in all 8 registry implementations - Add with_http_client() constructor to DependiBackend for client injection - Add tests verifying shared client instances across registries - Add debug assertions in initialized() to verify shared client This allows: - External code to verify which client registries are using - Dependency injection of custom HTTP clients for testing - Runtime verification that all registries share the same client
The doctests in registry modules were failing because they reference internal types without proper external crate imports. Doctests compile as external code, so they cannot use crate-internal paths. Marked all affected doctests with `ignore` attribute since these are illustrative examples that cannot be compiled in a doctest context.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @dependi-lsp/src/registries/crates_io.rs:
- Around line 12-13: Doctest examples are missing necessary imports, causing
external doctest failures; add the two imports at the top of the examples so the
examples can compile: import create_shared_client
(dependi_lsp::registries::http_client::create_shared_client) for the
with_client() example that calls create_shared_client() and
CratesIoRegistry::with_client(), and import CratesIoRegistry
(dependi_lsp::registries::crates_io::CratesIoRegistry) for both the
with_client() and Default examples that call CratesIoRegistry::with_client(...)
and CratesIoRegistry::default().
In @dependi-lsp/src/registries/npm.rs:
- Around line 4-11: Doctests for NpmRegistry are missing necessary imports;
update the with_client() doctest to include "use std::sync::Arc;", "use
reqwest::Client;", and "use dependi_lsp::registries::npm::NpmRegistry;" and
change the Arc usage to Arc::clone(&client) when calling
NpmRegistry::with_client; likewise add "use
dependi_lsp::registries::npm::NpmRegistry;" to the Default doctest so
NpmRegistry::default() runs in the external doctest context.
In @dependi-lsp/src/registries/packagist.rs:
- Around line 4-11: Doctests for PackagistRegistry are missing the necessary
import which breaks external-crate doctest compilation; update both the
with_client example (referencing PackagistRegistry::with_client and Arc<Client>)
and the Default impl example to include "use
dependi_lsp::registries::packagist::PackagistRegistry;" so the type is in scope
when running doctests as an external crate, leaving the rest of the examples
(Arc<Client> creation and create_shared_client/default usage) unchanged.
In @dependi-lsp/src/registries/pypi.rs:
- Around line 4-11: The doctest blocks for PyPiRegistry::with_client and the
Default impl are missing necessary imports and will fail to compile; update the
docs for the with_client example to include use std::sync::Arc; use
dependi_lsp::registries::http_client::create_shared_client; and use
dependi_lsp::registries::pypi::PyPiRegistry; before calling
create_shared_client() and PyPiRegistry::with_client(...), and update the
Default example to include use dependi_lsp::registries::pypi::PyPiRegistry; (or
alternatively mark both fenced code blocks as ```no_run``` if you intend them to
be illustrative).
🧹 Nitpick comments (3)
dependi-lsp/src/registries/http_client.rs (1)
54-93: Consider loosening theArc::strong_countassumptions in testsThe tests asserting
Arc::strong_count(&client) == 1and== 9tightly couple expectations to the exact number of clones performed today. Any future change (e.g., adding another registry or an extra clone inwith_client/http_client) will break these tests even if behavior remains correct. You might make these checks less brittle by:
- Using
assert!(Arc::strong_count(&shared_client) > 1)intest_registries_share_client_instance, and- Focusing on pointer equality (
Arc::ptr_eq) plus specific registry counts rather than an exact final strong count.dependi-lsp/src/backend.rs (2)
125-128: Add documentation for the public constructor.The
with_http_clientfunction is part of the public API but lacks documentation. Consider adding a docstring explaining the parameters, fallback behavior, and intended use cases.📝 Suggested docstring
+ /// Create a new DependiBackend with an optional shared HTTP client. + /// + /// # Arguments + /// + /// * `client` - The LSP client for communication + /// * `http_client` - Optional shared HTTP client. If None, a default shared client + /// will be created using `create_shared_client()`. + /// + /// # Panics + /// + /// Panics if `http_client` is None and `create_shared_client()` fails to create + /// a client (e.g., due to invalid TLS configuration). + /// + /// # Examples + /// + /// ``` + /// // With a custom HTTP client: + /// // let custom_client = create_custom_http_client(); + /// // let backend = DependiBackend::with_http_client(lsp_client, Some(custom_client)); + /// + /// // With default client: + /// // let backend = DependiBackend::with_http_client(lsp_client, None); + /// ``` pub fn with_http_client(client: Client, http_client: Option<Arc<HttpClient>>) -> Self {
143-150: Consider using consistent Arc cloning pattern.Lines 143-149 use
Arc::clone(&http_client)for all registries, but Line 150 passeshttp_clientdirectly forrubygems. While functionally correct (it's the last use), usingArc::clone(&http_client)would maintain consistency and make the code easier to understand at a glance.♻️ Proposed consistency fix
nuget: Arc::new(NuGetRegistry::with_client(Arc::clone(&http_client))), - rubygems: Arc::new(RubyGemsRegistry::with_client(http_client)), + rubygems: Arc::new(RubyGemsRegistry::with_client(Arc::clone(&http_client))), osv_client: Arc::new(OsvClient::default()),
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
dependi-lsp/src/backend.rsdependi-lsp/src/registries/crates_io.rsdependi-lsp/src/registries/go_proxy.rsdependi-lsp/src/registries/http_client.rsdependi-lsp/src/registries/mod.rsdependi-lsp/src/registries/npm.rsdependi-lsp/src/registries/nuget.rsdependi-lsp/src/registries/packagist.rsdependi-lsp/src/registries/pub_dev.rsdependi-lsp/src/registries/pypi.rsdependi-lsp/src/registries/rubygems.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- dependi-lsp/src/registries/go_proxy.rs
- dependi-lsp/src/registries/mod.rs
- dependi-lsp/src/registries/pub_dev.rs
🧰 Additional context used
🧬 Code graph analysis (6)
dependi-lsp/src/registries/crates_io.rs (8)
dependi-lsp/src/registries/go_proxy.rs (3)
http_client(65-67)with_client(32-37)default(50-52)dependi-lsp/src/registries/mod.rs (1)
http_client(142-142)dependi-lsp/src/registries/npm.rs (3)
http_client(127-129)with_client(33-38)default(49-51)dependi-lsp/src/registries/packagist.rs (3)
http_client(79-81)with_client(33-38)default(49-51)dependi-lsp/src/registries/pub_dev.rs (3)
http_client(80-82)with_client(31-36)default(47-49)dependi-lsp/src/registries/pypi.rs (3)
http_client(89-91)with_client(36-41)default(52-54)dependi-lsp/src/registries/rubygems.rs (3)
http_client(83-85)with_client(38-43)default(56-58)dependi-lsp/src/registries/http_client.rs (1)
create_shared_client(28-39)
dependi-lsp/src/registries/packagist.rs (2)
dependi-lsp/src/registries/pub_dev.rs (2)
http_client(80-82)with_client(31-36)dependi-lsp/src/registries/http_client.rs (1)
create_shared_client(28-39)
dependi-lsp/src/registries/pypi.rs (9)
dependi-lsp/src/registries/crates_io.rs (2)
http_client(114-116)with_client(60-66)dependi-lsp/src/registries/go_proxy.rs (2)
http_client(65-67)with_client(32-37)dependi-lsp/src/registries/mod.rs (1)
http_client(142-142)dependi-lsp/src/registries/npm.rs (2)
http_client(127-129)with_client(33-38)dependi-lsp/src/registries/nuget.rs (2)
http_client(98-100)with_client(30-35)dependi-lsp/src/registries/packagist.rs (2)
http_client(79-81)with_client(33-38)dependi-lsp/src/registries/pub_dev.rs (2)
http_client(80-82)with_client(31-36)dependi-lsp/src/registries/rubygems.rs (2)
http_client(83-85)with_client(38-43)dependi-lsp/src/registries/http_client.rs (1)
create_shared_client(28-39)
dependi-lsp/src/registries/http_client.rs (8)
dependi-lsp/src/registries/crates_io.rs (4)
new(22-27)with_client(60-66)http_client(114-116)default(84-86)dependi-lsp/src/registries/go_proxy.rs (3)
with_client(32-37)http_client(65-67)default(50-52)dependi-lsp/src/registries/npm.rs (3)
with_client(33-38)http_client(127-129)default(49-51)dependi-lsp/src/registries/nuget.rs (3)
with_client(30-35)http_client(98-100)default(48-50)dependi-lsp/src/registries/packagist.rs (3)
with_client(33-38)http_client(79-81)default(49-51)dependi-lsp/src/registries/pub_dev.rs (3)
with_client(31-36)http_client(80-82)default(47-49)dependi-lsp/src/registries/pypi.rs (3)
with_client(36-41)http_client(89-91)default(52-54)dependi-lsp/src/registries/rubygems.rs (3)
with_client(38-43)http_client(83-85)default(56-58)
dependi-lsp/src/backend.rs (10)
dependi-lsp/src/registries/crates_io.rs (3)
http_client(114-116)new(22-27)with_client(60-66)dependi-lsp/src/registries/go_proxy.rs (2)
http_client(65-67)with_client(32-37)dependi-lsp/src/registries/mod.rs (1)
http_client(142-142)dependi-lsp/src/registries/npm.rs (2)
http_client(127-129)with_client(33-38)dependi-lsp/src/registries/nuget.rs (2)
http_client(98-100)with_client(30-35)dependi-lsp/src/registries/packagist.rs (2)
http_client(79-81)with_client(33-38)dependi-lsp/src/registries/pub_dev.rs (2)
http_client(80-82)with_client(31-36)dependi-lsp/src/registries/pypi.rs (2)
http_client(89-91)with_client(36-41)dependi-lsp/src/registries/rubygems.rs (2)
http_client(83-85)with_client(38-43)dependi-lsp/src/registries/http_client.rs (1)
create_shared_client(28-39)
dependi-lsp/src/registries/rubygems.rs (6)
dependi-lsp/src/registries/crates_io.rs (1)
http_client(114-116)dependi-lsp/src/registries/mod.rs (1)
http_client(142-142)dependi-lsp/src/registries/npm.rs (1)
http_client(127-129)dependi-lsp/src/registries/pub_dev.rs (1)
http_client(80-82)dependi-lsp/src/registries/pypi.rs (1)
http_client(89-91)dependi-lsp/src/registries/http_client.rs (1)
create_shared_client(28-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (rust)
🔇 Additional comments (5)
dependi-lsp/src/registries/http_client.rs (1)
18-39: Shared HTTP client configuration looks solidTimeouts, pooling, and keepalive settings are wired correctly into a single
reqwest::Client, and returningArc<Client>cleanly supports reuse across registries. This factory function matches the PR’s shared-client objective well.dependi-lsp/src/registries/rubygems.rs (1)
13-14: RubyGems registry shared-client refactor and docs look good
RubyGemsRegistrycleanly adoptsArc<Client>withwith_client,Defaultwired throughcreate_shared_client(), and ahttp_client()accessor, consistent with the other registries. The doctest forwith_clientalso correctly demonstrates external usage viadependi_lsp::registries::rubygems::RubyGemsRegistry, so this file is in good shape.Also applies to: 22-43, 46-58, 82-85
dependi-lsp/src/backend.rs (3)
4-4: LGTM!The import additions are clean and necessary for the shared HTTP client implementation. The
HttpClientalias appropriately distinguishesreqwest::Clientfromtower_lsp::Client.Also applies to: 27-27
108-123: LGTM!The delegation to
with_http_clientmaintains backward compatibility while enabling the new shared client pattern. The docstring clearly explains the behavior and provides context.
697-707: Excellent defensive programming!The debug assertions verify the shared client invariant at runtime during development. Using
Arc::ptr_eqis the correct approach to confirm all registries reference the same underlying HTTP client, which is critical for the connection pooling benefits this PR aims to achieve.
1 participant
Summary
reqwest::Clientacross all 8 registry implementationsChanges
http_client.rsmodule withcreate_shared_client()functionArc<Client>viawith_client()constructorDefaultimpl for backward compatibilityTest plan
cargo clippy --all-targets -- -D warningspassescargo fmt --all -- --checkpassesCloses #31
Summary by CodeRabbit
New Features
Refactor
Stability
Tests / Documentation
✏️ Tip: You can customize this high-level summary in your review settings.