http-cache icon indicating copy to clipboard operation
http-cache copied to clipboard

Add hyper client middleware implementation

Open 06chaynes opened this issue 3 years ago • 10 comments

See docs

06chaynes avatar Jan 15 '22 15:01 06chaynes

"Shouldn't take long" haha you fool lol LMAO

If it happens that someone is actually waiting on/wanting this I'm still working on it in the background, in fact my attempts have informed a few changes in the core library... but every attempt has been met with issues that made the implementation straight up goofy.

Still making attempts though!

06chaynes avatar Feb 10 '22 22:02 06chaynes

The docs you've linked to seem to be the tower::Service implementation of hyper, so this is more a tower-http middleware than "proprietary" hyper-only client middleware, right?

I'm interested in building a custom HTTP client using tower-http, so I guess you can sign me up as wanting something like your crate for caching :) Is there some info on the diffuculties you've hit, e.g. upstream tracking issues or discussion?

robo9k avatar Mar 15 '22 17:03 robo9k

I had made some attempts locally but hadn't pushed anything upstream yet. I'll try to revisit it soon and collect more details on the issues I ran into. Think there were some question on error handling in a couple places (left using unwrap to press on at the time), as well as a lifetime issue in the Service implementation.

I think you're right about it being more of a tower::Service middleware rather than specific to hyper. I was just planning on using hyper for the client, but I'm open to any suggestions!

06chaynes avatar Mar 15 '22 18:03 06chaynes

I was planning on checking this out over the weekend but it turns out I have an addiction to Elden Ring, so my apologies on the delay

06chaynes avatar Mar 21 '22 17:03 06chaynes

@robo9k just pushed what changes I had committed on that attempt to a new branch 'hyper'. I apparently didn't commit my attempt of the Service implementation unfortunately, so only a mostly complete implementation of the http-cache Middleware trait is there.

06chaynes avatar Mar 22 '22 18:03 06chaynes

Making another attempt at this on the refactor branch, http-cache-tower Have a few issues to work out still though, like cloning the request body, getting rid of the unwraps, and how to best change things to run async functions inside Future::poll().

06chaynes avatar Aug 28 '22 19:08 06chaynes

Hey @06chaynes,

Let me know if you're looking for help to push this along! One idea that might help is to bump the version of http to 1.0.0, as both Request<B> and Response<B> now impl Clone if B: Clone. That also gets rid of most of the unwraps on your branch since you can pass req to the future directly rather than splitting it into parts and reconstituting.

I don't have much experience with http-cache itself, but I feel your pain around cloning the request body. Was working on an implementation of a tower service for an idempotency key cache a few months back and never made it past the request body cloning step.

In terms of how to run the async functions inside the poll, I wonder if adding a state enum that you match on inside poll could work? I found the implementation in tower-etag-cache really helpful when working on my idempotency cache: https://github.com/billythedummy/tower-etag-cache/blob/master/tower-etag-cache/src/future.rs

SebRollen avatar Jan 26 '24 01:01 SebRollen

@SebRollen any and all help is greatly welcomed. I haven't taken time to go over the http 1.0.0 changes yet but the addition of Clone to the request and response is absolutely fantastic to hear! I was planning on tackling that once Reqwest was updated to use the 1.0.0 release as well but I suppose it could happen before then.

I will for sure check over that implementation as I did run into issues trying to figure out how to get async functions working in a tower middleware, thanks again!

Another change I was trying to tackle soon was removing the HttpResponse struct and replacing it with a trait of the same name. This way I no longer need to convert back and forth between the response types. I made a pass at this change but ran into some issues that I'll need to think through, seems doable though.
Edit: Well thinking more on it I guess I couldn't make the last change I wanted as I wouldn't be able to implement the trait on the reqwest and surf response types, oh well was fun to try. I do think I can now remove the async-trait crate though which is something.

06chaynes avatar Jan 26 '24 02:01 06chaynes

@06chaynes great, happy to hear that! I put up a WIP PR to be able to discuss specific implementation questions. Let me know if you'd rather I branch it off of main, since the refactor branch is now quite far behind

SebRollen avatar Jan 26 '24 02:01 SebRollen

@SebRollen Sound good! Though I would recommend branching from develop as that refactor branch is way behind, I haven't made any changes to it for some time.

06chaynes avatar Jan 26 '24 03:01 06chaynes