Skip to content

Commit 453b2a8

Browse files
authored
perf: persist source client across requests (#95)
## What I'm changing In an effort to improve the data proxy's performance when communicating with the Source API, we can re-use our `reqwest::Client`. From the [`reqwest::Client` Docs](https://docs.rs/reqwest/latest/reqwest/struct.Client.html): > An asynchronous `Client` to make Requests with. > > ... > > The `Client` holds a connection pool internally, so it is advised that > you create one and **reuse** it. > > You do **not** have to wrap the `Client` in an [`Rc`] or [`Arc`] to **reuse** it, > because it already uses an [`Arc`] internally. ## How I did it Rather than creating a new client on each outbound call to the Source API, we instead create it once when we initialize the `SourceApi` struct and utilize it to make API calls when needed. <details> <summary>Summary of discussion with Claude</summary> ``` > Should we persists the reqwest client on the SourceApi struct for better performance? ⏺ Great question! Let me analyze the current implementation and the potential benefits. ⏺ Read(src/apis/source/mod.rs) ⎿ Read 50 lines ⏺ Search(pattern: "build_req_client", path: "src/apis/source/mod.rs", output_mode: "content") ⎿ Found 6 lines (ctrl+o to expand) ⏺ Read(src/apis/source/mod.rs) ⎿ Read 15 lines ⏺ Read(src/apis/source/mod.rs) ⎿ Read 45 lines Yes, absolutely! This would be a significant performance improvement. Let me explain why: Current Issues Looking at the code, build_req_client() is called 5 times across different API methods: - get_account (line 255) - get_repository_record (line 428) - fetch_data_connection (line 446) - fetch_api_key (line 511) - fetch_permission (line 591) Each call creates a brand new reqwest::Client, which means: Performance Problems 1. Lost Connection Pooling: reqwest::Client maintains an internal HTTP connection pool that reuses TCP connections. By creating a new client each time, you lose this major benefit - every request has to: - Perform a new DNS lookup - Establish a new TCP connection - Perform TLS handshake (for HTTPS) - Tear down the connection after use 2. Resource Overhead: Each client initialization has CPU and memory overhead 3. Against Best Practices: The reqwest documentation specifically recommends creating one client and reusing it Benefits of Persisting the Client 1. Connection Reuse: Subsequent requests to api.source.coop can reuse existing TCP connections (~20-50ms savings per request) 2. DNS Caching: The client caches DNS lookups internally 3. Better Resource Management: Fewer file descriptors, more efficient connection management 4. Thread Safe: reqwest::Client uses Arc internally, so it's cheap to clone and thread-safe - perfect for this struct that's already using Arc for caches Recommended Changes ╭─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ │ Ready to code? │ │ │ │ Here is Claude's plan: │ │ ╭─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ │ │ │ Persist reqwest::Client on SourceApi │ │ │ │ │ │ │ │ Changes to make: │ │ │ │ │ │ │ │ 1. Update SourceApi struct in src/apis/source/mod.rs: │ │ │ │ - Add client: reqwest::Client field (after proxy_url) │ │ │ │ - Add documentation explaining the client is reused for performance │ │ │ │ 2. Update SourceApi::new() constructor: │ │ │ │ - Build the reqwest::Client once during initialization │ │ │ │ - Store it in the struct instead of building it on every request │ │ │ │ 3. Update all API methods: │ │ │ │ - Replace 5 calls to self.build_req_client() with &self.client │ │ │ │ - Methods: get_account, get_repository_record, fetch_data_connection, fetch_api_key, fetch_permission │ │ │ │ 4. Remove build_req_client() method: │ │ │ │ - No longer needed since client is initialized once │ │ │ │ │ │ │ │ Performance Impact: │ │ │ │ - Enables HTTP connection pooling and reuse │ │ │ │ - Reduces latency by ~20-50ms per request (avoiding TCP handshake + TLS) │ │ │ │ - Better resource utilization │ │ │ │ - Follows reqwest best practices │ │ │ │ │ │ │ │ Risk: Very low - reqwest::Client is designed for this pattern and the struct is already Clone-compatible │ │ │ ╰─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ │ ``` </details> ## How to test it <!-- Inform the reviewer how they can validate that these changes work --> ## PR Checklist - [ ] This PR has **no** breaking changes. - [ ] I have updated or added new tests to cover the changes in this PR. - [ ] This PR affects the [Source Cooperative Frontend & API](https://github.com/source-cooperative/source.coop), and I have opened issue/PR #XXX to track the change. ## Related Issues <!-- Reference any existing related GitHub Issues -->
1 parent b25e8d1 commit 453b2a8

1 file changed

Lines changed: 22 additions & 35 deletions

File tree

src/apis/source/mod.rs

Lines changed: 22 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -119,8 +119,8 @@ pub struct SourceApi {
119119
/// Cache for user permissions
120120
permissions_cache: Arc<Cache<String, Vec<RepositoryPermission>>>,
121121

122-
/// Optional proxy URL for requests
123-
proxy_url: Option<String>,
122+
// API Client
123+
client: reqwest::Client,
124124
}
125125

126126
#[async_trait]
@@ -252,7 +252,6 @@ impl Api for SourceApi {
252252
account_id: String,
253253
user_identity: UserIdentity,
254254
) -> Result<Account, BackendError> {
255-
let client = self.build_req_client();
256255
// Create headers
257256
let mut headers = self.build_source_headers();
258257
if user_identity.api_key.is_some() {
@@ -266,7 +265,8 @@ impl Api for SourceApi {
266265
);
267266
}
268267

269-
let response = client
268+
let response = self
269+
.client
270270
.get(format!("{}/api/v1/products/{}", self.endpoint, account_id))
271271
.headers(headers)
272272
.send()
@@ -330,43 +330,33 @@ impl SourceApi {
330330
.build(),
331331
);
332332

333+
let client = {
334+
let mut client = reqwest::Client::builder()
335+
.user_agent(concat!("source-proxy/", env!("CARGO_PKG_VERSION")));
336+
if let Some(proxy) = proxy_url {
337+
client = client.proxy(reqwest::Proxy::all(proxy).unwrap());
338+
}
339+
client.build().unwrap()
340+
};
341+
333342
SourceApi {
334343
endpoint,
335344
api_key,
336345
product_cache,
337346
data_connection_cache,
338347
access_key_cache,
339348
permissions_cache,
340-
proxy_url,
349+
client,
341350
}
342351
}
343352

344-
/// Creates a new `reqwest::Client` with the appropriate proxy settings.
345-
///
346-
/// # Returns
347-
///
348-
/// Returns a `reqwest::Client` with the appropriate proxy settings.
349-
fn build_req_client(&self) -> reqwest::Client {
350-
let mut client = reqwest::Client::builder();
351-
if let Some(proxy) = &self.proxy_url {
352-
client = client.proxy(reqwest::Proxy::all(proxy).unwrap());
353-
}
354-
client.build().unwrap()
355-
}
356-
357353
/// Builds the headers for the Source API.
358354
///
359355
/// # Returns
360356
///
361357
/// Returns a `reqwest::header::HeaderMap` with the appropriate headers.
362358
fn build_source_headers(&self) -> reqwest::header::HeaderMap {
363-
const CORE_REQUEST_HEADERS: &[(&str, &str)] = &[
364-
("accept", "application/json"),
365-
(
366-
"user-agent",
367-
concat!("source-proxy/", env!("CARGO_PKG_VERSION")),
368-
),
369-
];
359+
const CORE_REQUEST_HEADERS: &[(&str, &str)] = &[("accept", "application/json")];
370360
CORE_REQUEST_HEADERS
371361
.iter()
372362
.map(|(name, value)| {
@@ -425,9 +415,8 @@ impl SourceApi {
425415
"{}/api/v1/products/{}/{}",
426416
self.endpoint, account_id, repository_id
427417
);
428-
let client = self.build_req_client();
429418
let headers = self.build_source_headers();
430-
let response = client.get(url).headers(headers).send().await?;
419+
let response = self.client.get(url).headers(headers).send().await?;
431420
let repository =
432421
process_json_response::<SourceProduct>(response, BackendError::RepositoryNotFound)
433422
.await?;
@@ -443,14 +432,14 @@ impl SourceApi {
443432
&self,
444433
data_connection_id: &str,
445434
) -> Result<DataConnection, BackendError> {
446-
let client = self.build_req_client();
447435
let mut headers = self.build_source_headers();
448436
headers.insert(
449437
reqwest::header::AUTHORIZATION,
450438
reqwest::header::HeaderValue::from_str(&self.api_key).unwrap(),
451439
);
452440

453-
let response = client
441+
let response = self
442+
.client
454443
.get(format!(
455444
"{}/api/v1/data-connections/{}",
456445
self.endpoint, data_connection_id
@@ -508,15 +497,14 @@ impl SourceApi {
508497
}
509498

510499
async fn fetch_api_key(&self, access_key_id: String) -> Result<APIKey, BackendError> {
511-
let client = self.build_req_client();
512-
513500
// Create headers
514501
let mut headers = self.build_source_headers();
515502
headers.insert(
516503
reqwest::header::AUTHORIZATION,
517504
reqwest::header::HeaderValue::from_str(&self.api_key).unwrap(),
518505
);
519-
let response = client
506+
let response = self
507+
.client
520508
.get(format!(
521509
"{}/api/v1/api-keys/{access_key_id}/auth",
522510
self.endpoint
@@ -588,8 +576,6 @@ impl SourceApi {
588576
account_id: &str,
589577
repository_id: &str,
590578
) -> Result<Vec<RepositoryPermission>, BackendError> {
591-
let client = self.build_req_client();
592-
593579
// Create headers
594580
let mut headers = self.build_source_headers();
595581
if user_identity.api_key.is_some() {
@@ -603,7 +589,8 @@ impl SourceApi {
603589
);
604590
}
605591

606-
let response = client
592+
let response = self
593+
.client
607594
.get(format!(
608595
"{}/api/v1/products/{account_id}/{repository_id}/permissions",
609596
self.endpoint

0 commit comments

Comments
 (0)