Stop leaking isahc assumption (#18408)

Users of our http_client crate knew they were interacting with isahc as
they set its extensions on the request. This change adds our own
equivalents for their APIs in preparation for changing the default http
client.

Release Notes:

- N/A
This commit is contained in:
Conrad Irwin 2024-09-26 14:01:05 -06:00 committed by GitHub
parent c1a039a5d7
commit e28496d4e2
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
24 changed files with 114 additions and 106 deletions

8
Cargo.lock generated
View file

@ -245,7 +245,6 @@ dependencies = [
"chrono",
"futures 0.3.30",
"http_client",
"isahc",
"schemars",
"serde",
"serde_json",
@ -2850,7 +2849,6 @@ dependencies = [
"gpui",
"http_client",
"indoc",
"isahc",
"language",
"lsp",
"menu",
@ -4128,7 +4126,6 @@ dependencies = [
"gpui",
"http_client",
"indexed_docs",
"isahc",
"isahc_http_client",
"language",
"log",
@ -4289,7 +4286,6 @@ dependencies = [
"gpui",
"http_client",
"human_bytes",
"isahc",
"language",
"log",
"menu",
@ -5016,7 +5012,6 @@ dependencies = [
"anyhow",
"futures 0.3.30",
"http_client",
"isahc",
"schemars",
"serde",
"serde_json",
@ -6288,7 +6283,6 @@ dependencies = [
"http_client",
"image",
"inline_completion_button",
"isahc",
"language",
"log",
"menu",
@ -7591,7 +7585,6 @@ dependencies = [
"anyhow",
"futures 0.3.30",
"http_client",
"isahc",
"schemars",
"serde",
"serde_json",
@ -14435,7 +14428,6 @@ dependencies = [
"image_viewer",
"inline_completion_button",
"install_cli",
"isahc",
"isahc_http_client",
"journal",
"language",

View file

@ -20,7 +20,6 @@ anyhow.workspace = true
chrono.workspace = true
futures.workspace = true
http_client.workspace = true
isahc.workspace = true
schemars = { workspace = true, optional = true }
serde.workspace = true
serde_json.workspace = true

View file

@ -6,9 +6,8 @@ use std::{pin::Pin, str::FromStr};
use anyhow::{anyhow, Context, Result};
use chrono::{DateTime, Utc};
use futures::{io::BufReader, stream::BoxStream, AsyncBufReadExt, AsyncReadExt, Stream, StreamExt};
use http_client::{AsyncBody, HttpClient, Method, Request as HttpRequest};
use isahc::config::Configurable;
use isahc::http::{HeaderMap, HeaderValue};
use http_client::http::{HeaderMap, HeaderValue};
use http_client::{AsyncBody, HttpClient, HttpRequestExt, Method, Request as HttpRequest};
use serde::{Deserialize, Serialize};
use strum::{EnumIter, EnumString};
use thiserror::Error;
@ -289,7 +288,7 @@ pub async fn stream_completion_with_rate_limit_info(
.header("X-Api-Key", api_key)
.header("Content-Type", "application/json");
if let Some(low_speed_timeout) = low_speed_timeout {
request_builder = request_builder.low_speed_timeout(100, low_speed_timeout);
request_builder = request_builder.read_timeout(low_speed_timeout);
}
let serialized_request =
serde_json::to_string(&request).context("failed to serialize request")?;

View file

@ -37,7 +37,6 @@ fs.workspace = true
futures.workspace = true
gpui.workspace = true
http_client.workspace = true
isahc.workspace = true
language.workspace = true
lsp.workspace = true
menu.workspace = true

View file

@ -7,8 +7,7 @@ use chrono::DateTime;
use fs::Fs;
use futures::{io::BufReader, stream::BoxStream, AsyncBufReadExt, AsyncReadExt, StreamExt};
use gpui::{AppContext, AsyncAppContext, Global};
use http_client::{AsyncBody, HttpClient, Method, Request as HttpRequest};
use isahc::config::Configurable;
use http_client::{AsyncBody, HttpClient, HttpRequestExt, Method, Request as HttpRequest};
use paths::home_dir;
use serde::{Deserialize, Serialize};
use settings::watch_config_file;
@ -275,7 +274,7 @@ async fn request_api_token(
.header("Accept", "application/json");
if let Some(low_speed_timeout) = low_speed_timeout {
request_builder = request_builder.low_speed_timeout(100, low_speed_timeout);
request_builder = request_builder.read_timeout(low_speed_timeout);
}
let request = request_builder.body(AsyncBody::empty())?;
@ -332,7 +331,7 @@ async fn stream_completion(
.header("Copilot-Integration-Id", "vscode-chat");
if let Some(low_speed_timeout) = low_speed_timeout {
request_builder = request_builder.low_speed_timeout(100, low_speed_timeout);
request_builder = request_builder.read_timeout(low_speed_timeout);
}
let request = request_builder.body(AsyncBody::from(serde_json::to_string(&request)?))?;
let mut response = client.send(request).await?;

View file

@ -28,7 +28,6 @@ futures.workspace = true
gpui.workspace = true
http_client.workspace = true
indexed_docs.workspace = true
isahc.workspace = true
language.workspace = true
log.workspace = true
lsp.workspace = true

View file

@ -664,7 +664,7 @@ impl ExtensionStore {
let content_length = response
.headers()
.get(isahc::http::header::CONTENT_LENGTH)
.get(http_client::http::header::CONTENT_LENGTH)
.and_then(|value| value.to_str().ok()?.parse::<usize>().ok());
let mut body = BufReader::new(response.body_mut());

View file

@ -1,5 +1,5 @@
use crate::wasm_host::{wit::ToWasmtimeResult, WasmState};
use ::http_client::AsyncBody;
use ::http_client::{AsyncBody, HttpRequestExt};
use ::settings::{Settings, WorktreeId};
use anyhow::{anyhow, bail, Context, Result};
use async_compression::futures::bufread::GzipDecoder;
@ -8,7 +8,6 @@ use async_trait::async_trait;
use futures::{io::BufReader, FutureExt as _};
use futures::{lock::Mutex, AsyncReadExt};
use indexed_docs::IndexedDocsDatabase;
use isahc::config::{Configurable, RedirectPolicy};
use language::{
language_settings::AllLanguageSettings, LanguageServerBinaryStatus, LspAdapterDelegate,
};
@ -297,10 +296,12 @@ fn convert_request(
let mut request = ::http_client::Request::builder()
.method(::http_client::Method::from(extension_request.method))
.uri(&extension_request.url)
.redirect_policy(match extension_request.redirect_policy {
http_client::RedirectPolicy::NoFollow => RedirectPolicy::None,
http_client::RedirectPolicy::FollowLimit(limit) => RedirectPolicy::Limit(limit),
http_client::RedirectPolicy::FollowAll => RedirectPolicy::Follow,
.follow_redirects(match extension_request.redirect_policy {
http_client::RedirectPolicy::NoFollow => ::http_client::RedirectPolicy::NoFollow,
http_client::RedirectPolicy::FollowLimit(limit) => {
::http_client::RedirectPolicy::FollowLimit(limit)
}
http_client::RedirectPolicy::FollowAll => ::http_client::RedirectPolicy::FollowAll,
});
for (key, value) in &extension_request.headers {
request = request.header(key, value);

View file

@ -1,5 +1,5 @@
use crate::wasm_host::{wit::ToWasmtimeResult, WasmState};
use ::http_client::AsyncBody;
use ::http_client::{AsyncBody, HttpRequestExt};
use ::settings::{Settings, WorktreeId};
use anyhow::{anyhow, bail, Context, Result};
use async_compression::futures::bufread::GzipDecoder;
@ -8,7 +8,6 @@ use async_trait::async_trait;
use futures::{io::BufReader, FutureExt as _};
use futures::{lock::Mutex, AsyncReadExt};
use indexed_docs::IndexedDocsDatabase;
use isahc::config::{Configurable, RedirectPolicy};
use language::{
language_settings::AllLanguageSettings, LanguageServerBinaryStatus, LspAdapterDelegate,
};
@ -213,10 +212,12 @@ fn convert_request(
let mut request = ::http_client::Request::builder()
.method(::http_client::Method::from(extension_request.method))
.uri(&extension_request.url)
.redirect_policy(match extension_request.redirect_policy {
http_client::RedirectPolicy::NoFollow => RedirectPolicy::None,
http_client::RedirectPolicy::FollowLimit(limit) => RedirectPolicy::Limit(limit),
http_client::RedirectPolicy::FollowAll => RedirectPolicy::Follow,
.follow_redirects(match extension_request.redirect_policy {
http_client::RedirectPolicy::NoFollow => ::http_client::RedirectPolicy::NoFollow,
http_client::RedirectPolicy::FollowLimit(limit) => {
::http_client::RedirectPolicy::FollowLimit(limit)
}
http_client::RedirectPolicy::FollowAll => ::http_client::RedirectPolicy::FollowAll,
});
for (key, value) in &extension_request.headers {
request = request.header(key, value);

View file

@ -23,7 +23,6 @@ editor.workspace = true
futures.workspace = true
gpui.workspace = true
human_bytes = "0.4.1"
isahc.workspace = true
http_client.workspace = true
language.workspace = true
log.workspace = true

View file

@ -11,7 +11,6 @@ use gpui::{
PromptLevel, Render, Task, View, ViewContext,
};
use http_client::HttpClient;
use isahc::Request;
use language::Buffer;
use project::Project;
use regex::Regex;
@ -299,7 +298,7 @@ impl FeedbackModal {
is_staff: is_staff.unwrap_or(false),
};
let json_bytes = serde_json::to_vec(&request)?;
let request = Request::post(feedback_endpoint)
let request = http_client::http::Request::post(feedback_endpoint)
.header("content-type", "application/json")
.body(json_bytes.into())?;
let mut response = http_client.send(request).await?;

View file

@ -3,7 +3,7 @@ use std::sync::Arc;
use anyhow::{bail, Context, Result};
use async_trait::async_trait;
use futures::AsyncReadExt;
use http_client::{AsyncBody, HttpClient, Request};
use http_client::{AsyncBody, HttpClient, HttpRequestExt, Request};
use serde::Deserialize;
use url::Url;
@ -49,14 +49,16 @@ impl Codeberg {
let url =
format!("https://codeberg.org/api/v1/repos/{repo_owner}/{repo}/git/commits/{commit}");
let mut request = Request::get(&url).header("Content-Type", "application/json");
let mut request = Request::get(&url)
.header("Content-Type", "application/json")
.follow_redirects(http_client::RedirectPolicy::FollowAll);
if let Ok(codeberg_token) = std::env::var("CODEBERG_TOKEN") {
request = request.header("Authorization", format!("Bearer {}", codeberg_token));
}
let mut response = client
.send_with_redirect_policy(request.body(AsyncBody::default())?, true)
.send(request.body(AsyncBody::default())?)
.await
.with_context(|| format!("error fetching Codeberg commit details at {:?}", url))?;

View file

@ -3,7 +3,7 @@ use std::sync::{Arc, OnceLock};
use anyhow::{bail, Context, Result};
use async_trait::async_trait;
use futures::AsyncReadExt;
use http_client::{AsyncBody, HttpClient, Request};
use http_client::{AsyncBody, HttpClient, HttpRequestExt, Request};
use regex::Regex;
use serde::Deserialize;
use url::Url;
@ -53,14 +53,16 @@ impl Github {
) -> Result<Option<User>> {
let url = format!("https://api.github.com/repos/{repo_owner}/{repo}/commits/{commit}");
let mut request = Request::get(&url).header("Content-Type", "application/json");
let mut request = Request::get(&url)
.header("Content-Type", "application/json")
.follow_redirects(http_client::RedirectPolicy::FollowAll);
if let Ok(github_token) = std::env::var("GITHUB_TOKEN") {
request = request.header("Authorization", format!("Bearer {}", github_token));
}
let mut response = client
.send_with_redirect_policy(request.body(AsyncBody::default())?, true)
.send(request.body(AsyncBody::default())?)
.await
.with_context(|| format!("error fetching GitHub commit details at {:?}", url))?;

View file

@ -18,7 +18,6 @@ schemars = ["dep:schemars"]
anyhow.workspace = true
futures.workspace = true
http_client.workspace = true
isahc.workspace = true
schemars = { workspace = true, optional = true }
serde.workspace = true
serde_json.workspace = true

View file

@ -2,8 +2,7 @@ mod supported_countries;
use anyhow::{anyhow, Result};
use futures::{io::BufReader, stream::BoxStream, AsyncBufReadExt, AsyncReadExt, Stream, StreamExt};
use http_client::{AsyncBody, HttpClient, Method, Request as HttpRequest};
use isahc::config::Configurable;
use http_client::{AsyncBody, HttpClient, HttpRequestExt, Method, Request as HttpRequest};
use serde::{Deserialize, Serialize};
use std::time::Duration;
@ -30,7 +29,7 @@ pub async fn stream_generate_content(
.header("Content-Type", "application/json");
if let Some(low_speed_timeout) = low_speed_timeout {
request_builder = request_builder.low_speed_timeout(100, low_speed_timeout);
request_builder = request_builder.read_timeout(low_speed_timeout);
};
let request = request_builder.body(AsyncBody::from(serde_json::to_string(&request)?))?;
@ -85,7 +84,7 @@ pub async fn count_tokens(
.header("Content-Type", "application/json");
if let Some(low_speed_timeout) = low_speed_timeout {
request_builder = request_builder.low_speed_timeout(100, low_speed_timeout);
request_builder = request_builder.read_timeout(low_speed_timeout);
}
let http_request = request_builder.body(AsyncBody::from(request))?;

View file

@ -1524,10 +1524,9 @@ pub struct KeystrokeEvent {
struct NullHttpClient;
impl HttpClient for NullHttpClient {
fn send_with_redirect_policy(
fn send(
&self,
_req: http_client::Request<http_client::AsyncBody>,
_follow_redirects: bool,
) -> futures::future::BoxFuture<
'static,
Result<http_client::Response<http_client::AsyncBody>, anyhow::Error>,

View file

@ -10,22 +10,46 @@ use futures::future::BoxFuture;
use http::request::Builder;
#[cfg(feature = "test-support")]
use std::fmt;
use std::sync::{Arc, Mutex};
use std::{
sync::{Arc, Mutex},
time::Duration,
};
pub use url::Url;
pub struct ReadTimeout(pub Duration);
#[derive(Default, Debug, Clone)]
pub enum RedirectPolicy {
#[default]
NoFollow,
FollowLimit(u32),
FollowAll,
}
pub struct FollowRedirects(pub bool);
pub trait HttpRequestExt {
/// Set a read timeout on the request.
/// For isahc, this is the low_speed_timeout.
/// For other clients, this is the timeout used for read calls when reading the response.
/// In all cases this prevents servers stalling completely, but allows them to send data slowly.
fn read_timeout(self, timeout: Duration) -> Self;
/// Whether or not to follow redirects
fn follow_redirects(self, follow: RedirectPolicy) -> Self;
}
impl HttpRequestExt for http::request::Builder {
fn read_timeout(self, timeout: Duration) -> Self {
self.extension(ReadTimeout(timeout))
}
fn follow_redirects(self, follow: RedirectPolicy) -> Self {
self.extension(follow)
}
}
pub trait HttpClient: 'static + Send + Sync {
fn send(
&self,
req: http::Request<AsyncBody>,
) -> BoxFuture<'static, Result<Response<AsyncBody>, anyhow::Error>> {
self.send_with_redirect_policy(req, false)
}
// TODO: Make a better API for this
fn send_with_redirect_policy(
&self,
req: Request<AsyncBody>,
follow_redirects: bool,
) -> BoxFuture<'static, Result<Response<AsyncBody>, anyhow::Error>>;
fn get<'a>(
@ -34,14 +58,17 @@ pub trait HttpClient: 'static + Send + Sync {
body: AsyncBody,
follow_redirects: bool,
) -> BoxFuture<'a, Result<Response<AsyncBody>, anyhow::Error>> {
let request = Builder::new().uri(uri).body(body);
let request = Builder::new()
.uri(uri)
.follow_redirects(if follow_redirects {
RedirectPolicy::FollowAll
} else {
RedirectPolicy::NoFollow
})
.body(body);
match request {
Ok(request) => Box::pin(async move {
self.send_with_redirect_policy(request, follow_redirects)
.await
.map_err(Into::into)
}),
Ok(request) => Box::pin(async move { self.send(request).await.map_err(Into::into) }),
Err(e) => Box::pin(async move { Err(e.into()) }),
}
}
@ -92,12 +119,11 @@ impl HttpClientWithProxy {
}
impl HttpClient for HttpClientWithProxy {
fn send_with_redirect_policy(
fn send(
&self,
req: Request<AsyncBody>,
follow_redirects: bool,
) -> BoxFuture<'static, Result<Response<AsyncBody>, anyhow::Error>> {
self.client.send_with_redirect_policy(req, follow_redirects)
self.client.send(req)
}
fn proxy(&self) -> Option<&Uri> {
@ -106,12 +132,11 @@ impl HttpClient for HttpClientWithProxy {
}
impl HttpClient for Arc<HttpClientWithProxy> {
fn send_with_redirect_policy(
fn send(
&self,
req: Request<AsyncBody>,
follow_redirects: bool,
) -> BoxFuture<'static, Result<Response<AsyncBody>, anyhow::Error>> {
self.client.send_with_redirect_policy(req, follow_redirects)
self.client.send(req)
}
fn proxy(&self) -> Option<&Uri> {
@ -218,12 +243,11 @@ impl HttpClientWithUrl {
}
impl HttpClient for Arc<HttpClientWithUrl> {
fn send_with_redirect_policy(
fn send(
&self,
req: Request<AsyncBody>,
follow_redirects: bool,
) -> BoxFuture<'static, Result<Response<AsyncBody>, anyhow::Error>> {
self.client.send_with_redirect_policy(req, follow_redirects)
self.client.send(req)
}
fn proxy(&self) -> Option<&Uri> {
@ -232,12 +256,11 @@ impl HttpClient for Arc<HttpClientWithUrl> {
}
impl HttpClient for HttpClientWithUrl {
fn send_with_redirect_policy(
fn send(
&self,
req: Request<AsyncBody>,
follow_redirects: bool,
) -> BoxFuture<'static, Result<Response<AsyncBody>, anyhow::Error>> {
self.client.send_with_redirect_policy(req, follow_redirects)
self.client.send(req)
}
fn proxy(&self) -> Option<&Uri> {
@ -283,14 +306,6 @@ impl HttpClient for BlockedHttpClient {
fn proxy(&self) -> Option<&Uri> {
None
}
fn send_with_redirect_policy(
&self,
req: Request<AsyncBody>,
_: bool,
) -> BoxFuture<'static, Result<Response<AsyncBody>, anyhow::Error>> {
self.send(req)
}
}
#[cfg(feature = "test-support")]
@ -352,10 +367,9 @@ impl fmt::Debug for FakeHttpClient {
#[cfg(feature = "test-support")]
impl HttpClient for FakeHttpClient {
fn send_with_redirect_policy(
fn send(
&self,
req: Request<AsyncBody>,
_follow_redirects: bool,
) -> BoxFuture<'static, Result<Response<AsyncBody>, anyhow::Error>> {
let future = (self.handler)(req);
future

View file

@ -1,7 +1,6 @@
use std::{mem, sync::Arc, time::Duration};
use futures::future::BoxFuture;
use isahc::config::RedirectPolicy;
use util::maybe;
pub use isahc::config::Configurable;
@ -36,18 +35,29 @@ impl HttpClient for IsahcHttpClient {
None
}
fn send_with_redirect_policy(
fn send(
&self,
req: http_client::http::Request<http_client::AsyncBody>,
follow_redirects: bool,
) -> BoxFuture<'static, Result<http_client::Response<http_client::AsyncBody>, anyhow::Error>>
{
let redirect_policy = req
.extensions()
.get::<http_client::RedirectPolicy>()
.cloned()
.unwrap_or_default();
let read_timeout = req
.extensions()
.get::<http_client::ReadTimeout>()
.map(|t| t.0);
let req = maybe!({
let (mut parts, body) = req.into_parts();
let mut builder = isahc::Request::builder()
.method(parts.method)
.uri(parts.uri)
.version(parts.version);
if let Some(read_timeout) = read_timeout {
builder = builder.low_speed_timeout(100, read_timeout);
}
let headers = builder.headers_mut()?;
mem::swap(headers, &mut parts.headers);
@ -64,10 +74,12 @@ impl HttpClient for IsahcHttpClient {
};
builder
.redirect_policy(if follow_redirects {
RedirectPolicy::Follow
} else {
RedirectPolicy::None
.redirect_policy(match redirect_policy {
http_client::RedirectPolicy::FollowAll => isahc::config::RedirectPolicy::Follow,
http_client::RedirectPolicy::FollowLimit(limit) => {
isahc::config::RedirectPolicy::Limit(limit)
}
http_client::RedirectPolicy::NoFollow => isahc::config::RedirectPolicy::None,
})
.body(isahc_body)
.ok()

View file

@ -32,7 +32,6 @@ futures.workspace = true
google_ai = { workspace = true, features = ["schemars"] }
gpui.workspace = true
http_client.workspace = true
isahc.workspace = true
inline_completion_button.workspace = true
log.workspace = true
menu.workspace = true

View file

@ -18,8 +18,7 @@ use gpui::{
AnyElement, AnyView, AppContext, AsyncAppContext, FontWeight, Model, ModelContext,
Subscription, Task,
};
use http_client::{AsyncBody, HttpClient, Method, Response};
use isahc::config::Configurable;
use http_client::{AsyncBody, HttpClient, HttpRequestExt, Method, Response};
use schemars::JsonSchema;
use serde::{de::DeserializeOwned, Deserialize, Serialize};
use serde_json::value::RawValue;
@ -396,7 +395,7 @@ impl CloudLanguageModel {
let response = loop {
let mut request_builder = http_client::Request::builder();
if let Some(low_speed_timeout) = low_speed_timeout {
request_builder = request_builder.low_speed_timeout(100, low_speed_timeout);
request_builder = request_builder.read_timeout(low_speed_timeout);
};
let request = request_builder
.method(Method::POST)

View file

@ -19,7 +19,6 @@ schemars = ["dep:schemars"]
anyhow.workspace = true
futures.workspace = true
http_client.workspace = true
isahc.workspace = true
schemars = { workspace = true, optional = true }
serde.workspace = true
serde_json.workspace = true

View file

@ -6,8 +6,7 @@ use futures::{
stream::{self, BoxStream},
AsyncBufReadExt, AsyncReadExt, Stream, StreamExt,
};
use http_client::{AsyncBody, HttpClient, Method, Request as HttpRequest};
use isahc::config::Configurable;
use http_client::{AsyncBody, HttpClient, HttpRequestExt, Method, Request as HttpRequest};
use serde::{Deserialize, Serialize};
use serde_json::Value;
use std::{
@ -318,7 +317,7 @@ pub async fn complete(
.header("Content-Type", "application/json")
.header("Authorization", format!("Bearer {}", api_key));
if let Some(low_speed_timeout) = low_speed_timeout {
request_builder = request_builder.low_speed_timeout(100, low_speed_timeout);
request_builder = request_builder.read_timeout(low_speed_timeout);
};
let mut request_body = request;
@ -413,7 +412,7 @@ pub async fn stream_completion(
.header("Authorization", format!("Bearer {}", api_key));
if let Some(low_speed_timeout) = low_speed_timeout {
request_builder = request_builder.low_speed_timeout(100, low_speed_timeout);
request_builder = request_builder.read_timeout(low_speed_timeout);
};
let request = request_builder.body(AsyncBody::from(serde_json::to_string(&request)?))?;

View file

@ -57,7 +57,6 @@ http_client.workspace = true
image_viewer.workspace = true
inline_completion_button.workspace = true
install_cli.workspace = true
isahc.workspace = true
isahc_http_client.workspace = true
journal.workspace = true
language.workspace = true

View file

@ -4,8 +4,7 @@ use chrono::Utc;
use client::telemetry;
use db::kvp::KEY_VALUE_STORE;
use gpui::{AppContext, SemanticVersion};
use http_client::Method;
use isahc::config::Configurable;
use http_client::{HttpRequestExt, Method};
use http_client::{self, HttpClient, HttpClientWithUrl};
use paths::{crashes_dir, crashes_retired_dir};
@ -491,7 +490,7 @@ async fn upload_previous_crashes(
.context("error reading crash file")?;
let mut request = http_client::Request::post(&crash_report_url.to_string())
.redirect_policy(isahc::config::RedirectPolicy::Follow)
.follow_redirects(http_client::RedirectPolicy::FollowAll)
.header("Content-Type", "text/plain");
if let Some((panicked_on, payload)) = most_recent_panic.as_ref() {