From e778b7df112bcce525020c2a4784c3e151158720 Mon Sep 17 00:00:00 2001 From: Ethan Brierley Date: Wed, 10 Apr 2024 09:30:17 +0100 Subject: [PATCH] chore: revert change switching from `Arc` to `Box` in public APIs (#142) fixes #139 There are other potential long term solutions listed [here](https://github.com/TrueLayer/reqwest-middleware/issues/139#issuecomment-2045946644) > I didn't put much thought into it. Internally there used to be a `Box<[Arc]>` which is cloned about quite often. > > * https://docs.rs/reqwest-middleware/0.2.5/src/reqwest_middleware/client.rs.html#87 > * https://docs.rs/reqwest-middleware/0.2.5/src/reqwest_middleware/client.rs.html#139 > > Because of all the cloning, I decided to flip it to be `Arc` instead of `Box`. > > I see three solutions here. > > 1. The one I'd lean towards is `Arc` even if it's a bit silly, but prevents extra unnecessary allocations on each request. > 2. An alternative is to implement Middleware for Arc and then you can use the regular with(...) api. This would mean there's an extra box around your middleware > 3. Revert this particular change In the short term, I think it's best to go with option 3. This will unblock the next release. We can consider the other options for future releasees --- CHANGELOG.md | 1 - reqwest-middleware/src/client.rs | 44 ++++++++++++++-------------- reqwest-middleware/src/middleware.rs | 6 ++-- 3 files changed, 26 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2055a08..d780818 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Updated `reqwest` to `0.12.0`. - Removed `task_local_extensions` in favour of `http::Extensions`. - All extensions must be `Clone` now. -- Replaced `with_arc` and `with_arc_init` methods with `with_box` and `with_box_init`. - Support for `matchit` `0.8.0` - This means router matches like `/a/:some_var` need to be changed to `/a/{some_var}` ### Changed diff --git a/reqwest-middleware/src/client.rs b/reqwest-middleware/src/client.rs index 8059618..4694531 100644 --- a/reqwest-middleware/src/client.rs +++ b/reqwest-middleware/src/client.rs @@ -18,8 +18,8 @@ use crate::RequestInitialiser; /// [`ClientWithMiddleware`]: crate::ClientWithMiddleware pub struct ClientBuilder { client: Client, - middleware_stack: Vec>, - initialiser_stack: Vec>, + middleware_stack: Vec>, + initialiser_stack: Vec>, } impl ClientBuilder { @@ -33,40 +33,40 @@ impl ClientBuilder { /// Convenience method to attach middleware. /// - /// If you need to keep a reference to the middleware after attaching, use [`with_box`]. + /// If you need to keep a reference to the middleware after attaching, use [`with_arc`]. /// - /// [`with_box`]: Self::with_box + /// [`with_arc`]: Self::with_arc pub fn with(self, middleware: M) -> Self where M: Middleware, { - self.with_box(Box::new(middleware)) + self.with_arc(Arc::new(middleware)) } - /// Add middleware to the chain. [`with`] is more ergonomic if you don't need the `Box`. + /// Add middleware to the chain. [`with`] is more ergonomic if you don't need the `Arc`. /// /// [`with`]: Self::with - pub fn with_box(mut self, middleware: Box) -> Self { + pub fn with_arc(mut self, middleware: Arc) -> Self { self.middleware_stack.push(middleware); self } /// Convenience method to attach a request initialiser. /// - /// If you need to keep a reference to the initialiser after attaching, use [`with_box_init`]. + /// If you need to keep a reference to the initialiser after attaching, use [`with_arc_init`]. /// - /// [`with_box_init`]: Self::with_box_init + /// [`with_arc_init`]: Self::with_arc_init pub fn with_init(self, initialiser: I) -> Self where I: RequestInitialiser, { - self.with_box_init(Box::new(initialiser)) + self.with_arc_init(Arc::new(initialiser)) } - /// Add a request initialiser to the chain. [`with_init`] is more ergonomic if you don't need the `Box`. + /// Add a request initialiser to the chain. [`with_init`] is more ergonomic if you don't need the `Arc`. /// /// [`with_init`]: Self::with_init - pub fn with_box_init(mut self, initialiser: Box) -> Self { + pub fn with_arc_init(mut self, initialiser: Arc) -> Self { self.initialiser_stack.push(initialiser); self } @@ -75,8 +75,8 @@ impl ClientBuilder { pub fn build(self) -> ClientWithMiddleware { ClientWithMiddleware { inner: self.client, - middleware_stack: self.middleware_stack.into(), - initialiser_stack: self.initialiser_stack.into(), + middleware_stack: self.middleware_stack.into_boxed_slice(), + initialiser_stack: self.initialiser_stack.into_boxed_slice(), } } } @@ -86,21 +86,21 @@ impl ClientBuilder { #[derive(Clone)] pub struct ClientWithMiddleware { inner: reqwest::Client, - middleware_stack: Arc<[Box]>, - initialiser_stack: Arc<[Box]>, + middleware_stack: Box<[Arc]>, + initialiser_stack: Box<[Arc]>, } impl ClientWithMiddleware { /// See [`ClientBuilder`] for a more ergonomic way to build `ClientWithMiddleware` instances. pub fn new(client: Client, middleware_stack: T) -> Self where - T: Into]>>, + T: Into]>>, { ClientWithMiddleware { inner: client, middleware_stack: middleware_stack.into(), // TODO(conradludgate) - allow downstream code to control this manually if desired - initialiser_stack: Arc::from(vec![]), + initialiser_stack: Box::new([]), } } @@ -222,8 +222,8 @@ impl From for ClientWithMiddleware { fn from(client: Client) -> Self { ClientWithMiddleware { inner: client, - middleware_stack: Arc::from(vec![]), - initialiser_stack: Arc::from(vec![]), + middleware_stack: Box::new([]), + initialiser_stack: Box::new([]), } } } @@ -314,8 +314,8 @@ mod service { #[must_use = "RequestBuilder does nothing until you 'send' it"] pub struct RequestBuilder { inner: reqwest::RequestBuilder, - middleware_stack: Arc<[Box]>, - initialiser_stack: Arc<[Box]>, + middleware_stack: Box<[Arc]>, + initialiser_stack: Box<[Arc]>, extensions: Extensions, } diff --git a/reqwest-middleware/src/middleware.rs b/reqwest-middleware/src/middleware.rs index 7230642..2854cf5 100644 --- a/reqwest-middleware/src/middleware.rs +++ b/reqwest-middleware/src/middleware.rs @@ -3,6 +3,8 @@ use reqwest::{Client, Request, Response}; use crate::error::{Error, Result}; +use std::sync::Arc; + /// When attached to a [`ClientWithMiddleware`] (generally using [`with`]), middleware is run /// whenever the client issues a request, in the order it was attached. /// @@ -73,7 +75,7 @@ where #[derive(Clone)] pub struct Next<'a> { client: &'a Client, - middlewares: &'a [Box], + middlewares: &'a [Arc], } #[cfg(not(target_arch = "wasm32"))] @@ -82,7 +84,7 @@ pub type BoxFuture<'a, T> = std::pin::Pin = std::pin::Pin + 'a>>; impl<'a> Next<'a> { - pub(crate) fn new(client: &'a Client, middlewares: &'a [Box]) -> Self { + pub(crate) fn new(client: &'a Client, middlewares: &'a [Arc]) -> Self { Next { client, middlewares,