-
Notifications
You must be signed in to change notification settings - Fork 5.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: don't share reqwest::HttpClient
across tokio runtimes
#24092
Conversation
@@ -641,140 +634,17 @@ impl FileFetcher { | |||
} | |||
} | |||
|
|||
#[derive(Debug, Eq, PartialEq)] | |||
enum FetchOnceResult { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to HttpClient
so that more of the reqwest
api usage is buried in there.
} | ||
} | ||
|
||
#[allow(clippy::print_stdout)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The below is mostly copy/paste from file_fetcher.rs
@@ -407,14 +518,67 @@ pub async fn get_response_body_with_progress( | |||
Ok(bytes.into()) | |||
} | |||
|
|||
/// Construct the next uri based on base uri and location header fragment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy+pste from file_fetcher.rs
// If there's an absolute url with no path, eg. https://my-cli.com | ||
// perform a request, and see if it redirects another file instead. | ||
let mut url = url.clone(); | ||
|
||
if url.path() == "/" { | ||
let client = HttpClient::new(None, None); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug. This was not taking into account the user's configuration.
@@ -672,9 +682,8 @@ async fn testify( | |||
}, | |||
}; | |||
|
|||
let client = Client::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug.
bail!("No OIDC token available"); | ||
}; | ||
|
||
let client = Client::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug.
rewest::HttpClient
across tokio runtimesreqwest::HttpClient
across tokio runtimes
cli/clippy.toml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thinking
I'm a bit wary about all these clones, but let's see if we see any regression in the benchmarks. |
Should be fine. It's only cloning Arcs and mostly only when making http requests ( |
This also fixes several issues where we weren't properly creating http clients with the user's settings.