Skip to content
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

Make ResponseFuture 'static #403

Closed
sagebind opened this issue Aug 5, 2022 · 2 comments · Fixed by #405
Closed

Make ResponseFuture 'static #403

sagebind opened this issue Aug 5, 2022 · 2 comments · Fixed by #405
Labels
enhancement Make a feature better
Milestone

Comments

@sagebind
Copy link
Owner

sagebind commented Aug 5, 2022

ResponseFuture is currently not 'static because it borrows the client instead of doing an implicit clone, but this can be annoying to use with a program structure where you don't pass around the client. This can probably be done without any additional clones or boxing if we do some internal restructuring of the client, which already uses an Arc, such that ResponseFuture can simply clone the same shared object that cloning an HttpClient itself does.

See also #401.

@sagebind sagebind added the enhancement Make a feature better label Aug 5, 2022
@sagebind sagebind added this to the 2.0.0 milestone Aug 5, 2022
@Xuanwo
Copy link

Xuanwo commented Aug 6, 2022

I'm interested in implementing this.

Is it OK to remove the lifetime directly? We can clone the client everytime we create a response future.

@sagebind
Copy link
Owner Author

sagebind commented Aug 6, 2022

Creating a ResponseFuture does already require cloning an Arc, but the scope of what that Arc contains is too small. We will want to broaden the scope such that only 1 clone is still required.

Internally the function that submits a request to the HTTP agent is called an Invoker, and the invoker is cloned here in the context whenever an interceptor is involved:

if let Some(interceptor) = self.interceptors.first() {
let inner_context = Self {
invoker: self.invoker.clone(),
interceptors: &self.interceptors[1..],
};
interceptor.intercept(request, inner_context).await
} else {
self.invoker.invoke(request).await
}

The invoker context is created and placed into an Arc here:

isahc/src/client.rs

Lines 1031 to 1034 in 2cb2fe4

let ctx = interceptor::Context {
invoker: Arc::new(self),
interceptors: &self.inner.interceptors,
};

To make ResponseFuture static, but without requiring holding onto two Arcs, we should get rid of the invoker Arc entirely and refactor the interceptor context to instead pass around the entire HttpClient. Since one field of a struct cannot borrow values from another, we'll need to change Context from using slices:

/// Execution context for an interceptor.
pub struct Context<'a> {
pub(crate) invoker: Arc<dyn Invoke + Send + Sync + 'a>,
pub(crate) interceptors: &'a [InterceptorObj],
}

to using indexes to keep track of which interceptor is the current one. That might look something like this:

pub struct Context { // note that Context can now be 'static
    pub(crate) client: HttpClient,
    pub(crate) interceptor_offset: usize,
}

As a side-effect, this refactoring may actually improve performance, since we won't need to heap-allocate an Invoker any more on every request.

sagebind pushed a commit that referenced this issue Nov 7, 2022
…ture (#405)

This PR will fix #403

The benchmark seems can't reflect the actual improvement. (I guess the test case(64K) is not suitable to measure the different between an `Arc::new`)

Before:

```shell
download 64K: isahc     time:   [201.80 ms 204.40 ms 207.00 ms]
```

After

```shell
download 64K: isahc     time:   [200.32 ms 203.57 ms 206.90 ms]
```

Signed-off-by: Xuanwo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Make a feature better
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants