Inject a custom reqwest::Client

[NobodyXu]

Summary 💡

Support someway of injecting reqwest::Client into gix, could be a builder method, or a global like with_reqwest_client, similar to how tracing/log does it (shame we don't have context in rustlang!)

Motivation 🔦

In cargo-binstall, we put a lot of configuration into reqwest::Client, including customised dns config.

Plus, having two clients means it cannot share the connection pool and dns record cache and dns connection pool.

https://github.com/cargo-bins/cargo-binstall/blob/c0ae9335b22cb4f297cdd1df049cd87bf2aa223f/crates/binstalk-downloader/src/remote.rs#L124

I know that gix use a blocking client and cargo-binstall uses an async one so one could argue sharing is not possible now, but looking at the reqwest blocking client source code it internally wraps an async one, so it should be trivial to add a TryFrom impl for async client to sync client

[NobodyXu]

cc Sebastian Thiel (@Byron) wdyt? Would it be reasonable to either have a tracing-like with_client or a builder, or some other way of injecting a client?

[Byron]

Thanks a lot for sharing. I think what we need here is twofold. First, there should be an async request transport and second it must be configurable so that the whole client is passed with the configuration.

The configuration system is fully dynamic and it should support passing a client already. But an async version of the request transport does not yet exist.

While that sounds like a lot of work, the real work these days is reviewing, because I'm pretty sure AI would do a great job at mostly copy pasting existing implementation and mixing them so that it makes sense. It's just not anything that I can pick up anytime soon because I'm review-bottlenecked already.

[NobodyXu]

Do you have any picture of how it should look? Is it just an async version of what's under gix_transport::client::blocking_io I can have a try when I'm free

For async/sync transport AFAIK previously it's stuck on the maybe-async can only generate one version of trait issue

[Byron]

For async/sync transport AFAIK previously it's stuck on the maybe-async can only generate one version of trait issue

This shouldn't be an issue anymore (or I completely forgot about it, which is a possibility).
And while setting a blocking client is totally possible (see below, skim-checked by me), there is still no async http implementation. But I'd be surprised if that would be too cumbersome to do nowadays. But since it's async, I may be very wrong as well.

Codex

I looked through the transport configuration path, and the current shape already seems close to supporting this.

Relevant code refs from ab2fee14651202fcb7b3d8178932090c73492014:

• Transport configuration is intentionally dynamic already: TransportWithoutIO::configure(&dyn Any):
  

  gitoxide/gix-transport/src/client/traits.rs

  Lines 48 to 51 in ab2fee1

  	/// Pass `config` can be cast and interpreted by the implementation, as documented separately.
  	///
  	/// The caller must know how that `config` data looks like for the intended implementation.
  	fn configure(&mut self, config: &dyn Any) -> Result<(), Box<dyn std::error::Error + Send + Sync + 'static>>;

• Repository::transport_options() returns Option<Box<dyn Any>>, and its docs explicitly say callers may downcast and modify it before passing it to the transport:
  

  gitoxide/gix/src/repository/config/transport.rs

  Lines 7 to 13 in ab2fee1

  	/// Produce configuration suitable for `url`, as differentiated by its protocol/scheme, to be passed to a transport instance via
  	/// [configure()][gix_transport::client::TransportWithoutIO::configure()] (via `&**config` to pass the contained `Any` and not the `Box`).
  	/// `None` is returned if there is no known configuration. If `remote_name` is not `None`, the remote's name may contribute to
  	/// configuration overrides, typically for the HTTP transport.
  	///
  	/// Note that the caller may cast the instance themselves to modify it before passing it on.
  	///

• Connection::with_transport_options() / set_transport_options() already accept a Box<dyn Any>:
  

  gitoxide/gix/src/remote/connection/access.rs

  Lines 34 to 44 in ab2fee1

  	/// Provide configuration to be used before the first handshake is conducted.
  	/// It's typically created by initializing it with [`Repository::transport_options()`](crate::Repository::transport_options()),
  	/// which is also the default if this isn't set explicitly. Note that all the default configuration is created from `git`
  	/// configuration, which can also be manipulated through overrides to affect the default configuration.
  	///
  	/// Use this method to provide transport configuration with custom backend configuration that is not configurable by other means and
  	/// custom to the application at hand.
  	pub fn with_transport_options(mut self, config: Box<dyn std::any::Any>) -> Self {
  	self.transport_options = Some(config);
  	self
  	}

  
  

  gitoxide/gix/src/remote/connection/access.rs

  Lines 61 to 65 in ab2fee1

  	/// Like [`with_transport_options()`](Self::with_transport_options()), but without consuming the connection.
  	pub fn set_transport_options(&mut self, config: Box<dyn std::any::Any>) -> &mut Self {
  	self.transport_options = Some(config);
  	self
  	}

• Those options are applied automatically before the handshake:
  

  gitoxide/gix/src/remote/connection/ref_map.rs

  Lines 134 to 145 in ab2fee1

  	let repo = self.remote.repo;
  	if self.transport_options.is_none() {
  	self.transport_options = repo
  	.transport_options(url.as_ref(), self.remote.name().map(crate::remote::Name::as_bstr))
  	.map_err(|err| Error::GatherTransportConfig {
  	source: err,
  	url: url.into_owned(),
  	})?;
  	}
  	if let Some(config) = self.transport_options.as_ref() {
  	self.transport.inner.configure(&**config)?;
  	}

• HTTP options already have a backend-specific escape hatch:
  

  gitoxide/gix-transport/src/client/blocking_io/http/mod.rs

  Lines 187 to 188 in ab2fee1

  	/// Backend specific options, if available.
  	pub backend: Option<Arc<Mutex<dyn Any + Send + Sync + 'static>>>,

• curl already uses that backend-specific field for curl-only options:
  

  gitoxide/gix/src/repository/config/transport.rs

  Lines 427 to 440 in ab2fee1

  	#[cfg(feature = "blocking-http-transport-curl")]
  	{
  	let key = "http.schannelCheckRevoke";
  	let schannel_check_revoke = config
  	.boolean_filter(key, &mut trusted_only)
  	.map(|value| config::tree::Http::SCHANNEL_CHECK_REVOKE.enrich_error(value))
  	.transpose()
  	.with_leniency(lenient)
  	.map_err(config::transport::http::Error::from)?;
  	let backend =
  	gix_protocol::transport::client::blocking_io::http::curl::Options { schannel_check_revoke };
  	opts.backend =
  	Some(Arc::new(Mutex::new(backend)) as Arc<Mutex<dyn Any + Send + Sync + 'static>>);
  	}

• The reqwest backend already has its own backend-specific Options, currently only configure_request:
  

  gitoxide/gix-transport/src/client/blocking_io/http/reqwest/mod.rs

  Lines 20 to 25 in ab2fee1

  	/// Options to configure the reqwest HTTP handler.
  	#[derive(Default)]
  	pub struct Options {
  	/// A function to configure the request that is about to be made.
  	pub configure_request: Option<Box<ConfigureRequestFn>>,
  	}

• And it already downcasts http::Options::backend to those reqwest options per request:
  

  gitoxide/gix-transport/src/client/blocking_io/http/reqwest/remote.rs

  Lines 126 to 133 in ab2fee1

  	let mut req = req_builder.build()?;
  	if let Some(ref mut request_options) = config.backend.as_ref().and_then(|backend| backend.lock().ok()) {
  	if let Some(options) = request_options.downcast_mut::<super::Options>() {
  	if let Some(configure_request) = &mut options.configure_request {
  	configure_request(&mut req)?;
  	}
  	}
  	}

So for the existing blocking reqwest transport, a small extension could be:

pub struct Options {
    pub configure_request: Option<Box<ConfigureRequestFn>>,
    pub client: Option<reqwest::blocking::Client>,
}

Then Remote would initialize its worker thread from the provided blocking client if present, falling back to the current builder-created client. The caller would pass it through the existing path:

let mut opts = repo
    .transport_options(url, remote_name)?
    .expect("http transport options");

let http = opts
    .downcast_mut::<gix_transport::client::blocking_io::http::Options>()
    .expect("http options");

http.backend = Some(Arc::new(Mutex::new(
    gix_transport::client::blocking_io::http::reqwest::Options {
        configure_request: None,
        client: Some(reqwest::blocking::Client::builder().build()?),
    },
)));

connection.set_transport_options(opts);

For clone, PrepareFetch::configure_connection() is probably the public hook to do that:

gitoxide/gix/src/clone/mod.rs

Lines 150 to 161 in ab2fee1

	/// Set a callback to use for configuring the connection to use right before connecting to the remote.
	///
	/// It is most commonly used for custom configuration.
	// TODO: tests
	pub fn configure_connection(
	mut self,
	f: impl FnMut(
	&mut crate::remote::Connection<'_, '_, Box<dyn Transport + Send>>,
	) -> Result<(), Box<dyn std::error::Error + Send + Sync>>
	+ 'static,
	) -> Self {
	self.configure_connection = Some(Box::new(f));

The important caveat is about passing an async reqwest::Client.

Today, the blocking reqwest backend creates a reqwest::blocking::Client inside its own worker thread:

gitoxide/gix-transport/src/client/blocking_io/http/reqwest/remote.rs

Lines 39 to 85 in ab2fee1

	impl Default for Remote {
	fn default() -> Self {
	let (req_send, req_recv) = std::sync::mpsc::sync_channel(0);
	let (res_send, res_recv) = std::sync::mpsc::sync_channel(0);
	let handle = std::thread::spawn(move || -> Result<(), Error> {
	let mut follow = None;
	let mut redirected_base_url = None::<String>;
	let allow_redirects = Arc::new(atomic::AtomicBool::new(false));
	// We may error while configuring, which is expected as part of the internal protocol. The error will be
	// received and the sender of the request might restart us.
	let client = reqwest::blocking::ClientBuilder::new()
	.connect_timeout(std::time::Duration::from_secs(20))
	.http1_title_case_headers()
	.redirect(reqwest::redirect::Policy::custom({
	let allow_redirects = allow_redirects.clone();
	move |attempt| {
	if allow_redirects.load(atomic::Ordering::Relaxed) {
	let curr_url = attempt.url();
	let prev_urls = attempt.previous();
	match prev_urls.first() {
	Some(prev_url)
	if !redirect::shares_authority_or_upgrades_scheme(
	curr_url.as_str(),
	prev_url.as_str(),
	) =>
	{
	// git does not want to be redirected to a different host.
	attempt.stop()
	}
	_ => {
	// emulate default git behaviour which relies on curl default behaviour apparently.
	const CURL_DEFAULT_REDIRS: usize = 50;
	if prev_urls.len() >= CURL_DEFAULT_REDIRS {
	attempt.error("too many redirects")
	} else {
	attempt.follow()
	}
	}
	}
	} else {
	attempt.stop()
	}
	}
	}))
	.build()?;

Reqwest’s blocking client internally uses an async builder/runtime, but in reqwest 0.13.2 I only see conversion from async_impl::ClientBuilder to blocking::ClientBuilder, not from an already-built async reqwest::Client to a blocking client. So passing a fully built async client, preserving its pool/DNS state, seems like it should be modeled as an async HTTP transport rather than as a blocking transport option.

In other words, I’d split this into two layers:

1. Short-term/blocking: allow injecting a reqwest::blocking::Client through the existing http::Options::backend mechanism.
2. Proper async support: add an async HTTP transport, probably mirroring client::blocking_io::http::{Transport, traits::Http} under client::async_io::http, whose reqwest backend accepts an entire reqwest::Client.

That second path would let downstreams like cargo-binstall pass their real async client and share connection pools, DNS cache, TLS config, proxy setup, etc., without trying to force it through reqwest’s blocking wrapper.

[NobodyXu]

Today, the blocking reqwest backend creates a reqwest::blocking::Client inside its own worker thread:

I took a look at it, and I think it'd actually be possible to add a new implementation to create blocking client from async? That'd be the simplest path if maintainer of reqwest, seanmonstar accepts it

Or maybe we should allow a generic implementation through trait and injected through options, something like:

struct HttpTransportBackend(Box<dyn Http<
    Headers = Box<dyn BufRead + Unpin>,
    ResponseBody = Box<dyn BufRead>,
    PostBody = Box<dyn Write>,
>>);

If you think the first solution is better I can try and open a PR to reqwest

[Byron]

If you think the first solution is better I can try and open a PR to reqwest

I'd definitely prefer that, thank you!

[NobodyXu]

Opened seanmonstar/reqwest#3024