isahc icon indicating copy to clipboard operation
isahc copied to clipboard

Make `ResponseFuture` `'static`

Open sagebind opened this issue 1 year ago • 2 comments

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 https://github.com/sagebind/isahc/discussions/401.

sagebind avatar Aug 05 '22 22:08 sagebind

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.

Xuanwo avatar Aug 06 '22 16:08 Xuanwo

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:

https://github.com/sagebind/isahc/blob/2cb2fe4b1fecd33377e579596f45577380991067/src/interceptor/context.rs#L16-L25

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

https://github.com/sagebind/isahc/blob/2cb2fe4b1fecd33377e579596f45577380991067/src/client.rs#L1031-L1034

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:

https://github.com/sagebind/isahc/blob/2cb2fe4b1fecd33377e579596f45577380991067/src/interceptor/context.rs#L6-L10

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 avatar Aug 06 '22 17:08 sagebind