Skip to content

Commit 6307456

Browse files
authored
Convert GitHub fast path to use http_async (#16912)
### What does this PR try to resolve? - Add a new `request_blocking` function to the `http_async` module. This function does not start an executor and runs the request on the current thread, preventing #16860 - Migrates the GitHub fastpath to use the blocking http_async client. cc #16845 ### How to test and review this PR? Commit by commit. All commits pass tests.
2 parents 72d80ec + 810cb7e commit 6307456

File tree

2 files changed

+53
-53
lines changed

2 files changed

+53
-53
lines changed

src/cargo/sources/git/utils.rs

Lines changed: 13 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,8 @@ use crate::util::{GlobalContext, IntoUrl, MetricsCounter, Progress, network};
1414
use anyhow::{Context as _, anyhow};
1515
use cargo_util::{ProcessBuilder, paths};
1616
use cargo_util_terminal::Verbosity;
17-
use curl::easy::List;
1817
use git2::{ErrorClass, ObjectType, Oid};
18+
use http::{Request, StatusCode};
1919
use tracing::{debug, info};
2020
use url::Url;
2121

@@ -1580,36 +1580,21 @@ fn github_fast_path(
15801580
"https://api.github.com/repos/{}/{}/commits/{}",
15811581
username, repository, github_branch_name,
15821582
);
1583-
let mut handle = gctx.http()?.lock().unwrap();
15841583
debug!("attempting GitHub fast path for {}", url);
1585-
handle.get(true)?;
1586-
handle.url(&url)?;
1587-
handle.useragent("cargo")?;
1588-
handle.follow_location(true)?; // follow redirects
1589-
handle.http_headers({
1590-
let mut headers = List::new();
1591-
headers.append("Accept: application/vnd.github.3.sha")?;
1592-
if let Some(local_object) = local_object {
1593-
headers.append(&format!("If-None-Match: \"{}\"", local_object))?;
1594-
}
1595-
headers
1596-
})?;
1597-
1598-
let mut response_body = Vec::new();
1599-
let mut transfer = handle.transfer();
1600-
transfer.write_function(|data| {
1601-
response_body.extend_from_slice(data);
1602-
Ok(data.len())
1603-
})?;
1604-
transfer.perform()?;
1605-
drop(transfer); // end borrow of handle so that response_code can be called
1606-
1607-
let response_code = handle.response_code()?;
1608-
if response_code == 304 {
1584+
let mut request =
1585+
Request::get(url).header(http::header::ACCEPT, "application/vnd.github.3.sha");
1586+
if let Some(local_object) = local_object {
1587+
request = request.header(http::header::IF_NONE_MATCH, &format!("\"{local_object}\""));
1588+
}
1589+
let response = gctx
1590+
.http_async()?
1591+
.request_blocking(request.body(Vec::new())?)?;
1592+
let response_code = response.status();
1593+
if response_code == StatusCode::NOT_MODIFIED {
16091594
debug!("github fast path up-to-date");
16101595
Ok(FastPathRev::UpToDate)
1611-
} else if response_code == 200
1612-
&& let Some(oid_to_fetch) = rev_to_oid(str::from_utf8(&response_body)?)
1596+
} else if response_code == StatusCode::OK
1597+
&& let Some(oid_to_fetch) = rev_to_oid(str::from_utf8(&response.body())?)
16131598
{
16141599
// response expected to be a full hash hexstring (40 or 64 chars)
16151600
debug!("github fast path fetch {oid_to_fetch}");

src/cargo/util/network/http_async.rs

Lines changed: 40 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -95,8 +95,27 @@ impl Client {
9595
}
9696
}
9797

98+
/// Perform a blocking HTTP request using this client.
99+
/// Does not start an async executor.
100+
pub fn request_blocking(&self, request: Request) -> HttpResult<Response> {
101+
let handle = self.request_helper(request)?;
102+
handle.perform()?;
103+
Ok(WorkerServer::process_response(handle))
104+
}
105+
98106
/// Perform an HTTP request using this client.
99107
pub async fn request(&self, request: Request) -> HttpResult<Response> {
108+
let handle = self.request_helper(request)?;
109+
let (sender, receiver) = oneshot::channel();
110+
let req = Message {
111+
easy: handle,
112+
sender,
113+
};
114+
self.channel.as_ref().unwrap().send(req).unwrap();
115+
receiver.await.unwrap()
116+
}
117+
118+
fn request_helper(&self, request: Request) -> HttpResult<Easy2<Collector>> {
100119
let url = request.uri().to_string();
101120
debug!(target: "network::fetch", url);
102121
let mut collector = Collector::new(self.stats.clone());
@@ -141,14 +160,7 @@ impl Client {
141160
}
142161
handle.http_headers(headers)?;
143162

144-
let (sender, receiver) = oneshot::channel();
145-
let req = Message {
146-
easy: handle,
147-
sender,
148-
};
149-
150-
self.channel.as_ref().unwrap().send(req).unwrap();
151-
receiver.await.unwrap()
163+
Ok(handle)
152164
}
153165

154166
/// Returns the number pending bytes across all active transfers.
@@ -242,6 +254,24 @@ impl WorkerServer {
242254
}
243255
}
244256

257+
fn process_response(mut easy: Easy2<Collector>) -> Response {
258+
let mut response =
259+
std::mem::replace(&mut easy.get_mut().response, Response::new(Vec::new()));
260+
if let Ok(status) = easy.response_code()
261+
&& status != 0
262+
&& let Ok(status) = http::StatusCode::from_u16(status as u16)
263+
{
264+
*response.status_mut() = status;
265+
}
266+
// Would be nice to set HTTP version via `response.version_mut()`, but `curl` doesn't have it exposed.
267+
let extensions = Extensions {
268+
client_ip: easy.primary_ip().ok().flatten().map(str::to_string),
269+
effective_url: easy.effective_url().ok().flatten().map(str::to_string),
270+
};
271+
response.extensions_mut().insert(extensions);
272+
response
273+
}
274+
245275
/// Marks the start of a new timeout window.
246276
fn reset_low_speed_timeout(&mut self) {
247277
self.low_speed_window_start = Instant::now();
@@ -297,23 +327,8 @@ impl WorkerServer {
297327
return;
298328
};
299329
let result = msg.result_for2(&handle).expect("handle must have a result");
300-
let mut easy = self.multi.remove2(handle).expect("handle must be in multi");
301-
let mut response = std::mem::replace(
302-
&mut easy.get_mut().response,
303-
Response::new(Vec::new()),
304-
);
305-
if let Ok(status) = easy.response_code()
306-
&& status != 0
307-
&& let Ok(status) = http::StatusCode::from_u16(status as u16)
308-
{
309-
*response.status_mut() = status;
310-
}
311-
// Would be nice to set HTTP version via `response.version_mut()`, but `curl` doesn't have it exposed.
312-
let extensions = Extensions {
313-
client_ip: easy.primary_ip().ok().flatten().map(str::to_string),
314-
effective_url: easy.effective_url().ok().flatten().map(str::to_string),
315-
};
316-
response.extensions_mut().insert(extensions);
330+
let easy = self.multi.remove2(handle).expect("handle must be in multi");
331+
let response = Self::process_response(easy);
317332
let _ = sender.send(result.map(|()| response).map_err(Into::into));
318333
});
319334

0 commit comments

Comments
 (0)