-
Notifications
You must be signed in to change notification settings - Fork 133
Support for response caching #36
Comments
Good idea! Wonder if I can leverage something existing to do the work i.e. CacheCow |
I know Ali (the world is so small ;-) )
That would be cool - the only question is how to handle the dependency.
Separate NuGet package maybe?
…On Fri, 25 Jan 2019, 18:26 Damian Hickey ***@***.*** wrote:
Good idea! Wonder if I can leverage something existing to do the work i.e.
CacheCow
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#36 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABsWj1TirXTezoMANmKoQgWZWSFNLw4Hks5vG0xJgaJpZM4aQ5DT>
.
|
I've done a spike and the caching can be simply implemented using:
Since CacheCow supports using I would like to live it to you to decide on the best integration design. |
Good start. The small problem with this is that is it not using the HttpClient factory. |
I think I'll go along with recipe right now as opposed to maintaining integration packages. |
I assume, Damian, that you would like CacheCow to use the HttpClient instance injected into ForwardingContext instead of using own instance? |
I've just noticed that ProxyMiddleware doesn't seem to be using HttpClientFactory provided in the constructor. Is this by design? |
No, that's dead code that I forgot to remove (I'll remove it now). The code of interest is: |
Does it mean that if I register HttpClient with CacheCow caching handler
than it should be picked up here and used by ProxyKit?
Or am I missing something?
…On Sat, 26 Jan 2019, 11:36 Damian Hickey ***@***.*** wrote:
No, that's dead code that I forgot to remove (I'll remove it now). The
code of interest is:
https://github.com/damianh/ProxyKit/blob/3b1f34d072b13e60ed639d1f7edf6b5c9c14160f/src/ProxyKit/ProxyContextExtensions.cs#L31-L33
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#36 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABsWj5q8ACQXlzievYx_xG_Nb6Bq55wXks5vHD3ZgaJpZM4aQ5DT>
.
|
The way HttpClientFactory works is by disposing and re-creating all the handlers on a timed interval. I have a funny feeling that ProxyKit's API as it currently is might not quite be as simple as desired. |
Hooking up a static instance of
https://github.com/aliostad/CacheCow/blob/master/src/CacheCow.Client/CachingHandler.cs
should do the job, shouldn't it?
…On Sat, 26 Jan 2019, 12:00 Damian Hickey ***@***.*** wrote:
SharedProxyOptions has a place to insert a custom HttpMessageHandler that
is used when creating the HttpMessageHandler pipeline when creating the
HttpClient that is injected into ProxyKitClient (aka "typed http client"
<https://www.hanselman.com/blog/HttpClientFactoryForTypedHttpClientInstancesInASPNETCore21.aspx>).
Am going to see if it's easy to inject CachingHandler here.
The way HttpClientFactory works is by disposing and re-creating all the
handlers on a timed interval. I have a funny feeling that ProxyKit's API as
it currently is might not quite be as simple as desired.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#36 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABsWjy6PmZ2opSad2khZWe9HwvlwtHQRks5vHENxgaJpZM4aQ5DT>
.
|
Can't be static. HttpClientFactory will want to dispose it and recreate it. This can result in |
Ok. There is an overload that takes a Cache store. I can create a single
static instance of the store but keep recreating the handler itself.
…On Sat, 26 Jan 2019, 12:12 Damian Hickey ***@***.*** wrote:
Can't be static. HttpClientFactory will want to dispose it and recreate
it. This can result in ObjectDisposedExceptions a couple of minutes into
a running system (which doesn't show up in tests). Related #8
<#8>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#36 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABsWj0q2CDrgRZoxROGG6bWyPFAZnsrDks5vHEZLgaJpZM4aQ5DT>
.
|
Yes, singleton CacheStore |
So this appears to work now: var cacheStore = new InMemoryCacheStore(TimeSpan.FromMinutes(1));
services.AddProxy(options =>
{
options.GetMessageHandler = () => new CachingHandler(cacheStore)
{
InnerHandler = new HttpClientHandler {AllowAutoRedirect = false, UseCookies = false}
};
}); Not quite pit-of-success...
Need to think about this API a bit. Example CacheCow test code here: https://github.com/damianh/ProxyKit/blob/cachecow/src/ProxyKit.Tests/CacheCowTests.cs#L19 |
Just came up with the same myself ;-)
I don't really mind about not using DI for cache store - the whole proxy
thanks to your lib fits on one screen :-D
…On Sat, 26 Jan 2019, 12:31 Damian Hickey ***@***.*** wrote:
So this appears to work now:
var cacheStore = new InMemoryCacheStore(TimeSpan.FromMinutes(1));services.AddProxy(options =>
{
options.GetMessageHandler = () => new CachingHandler(cacheStore)
{
InnerHandler = new HttpClientHandler {AllowAutoRedirect = false, UseCookies = false}
};
});
Not quite pit-of-success...
1. options.GetMessageHandler is hooked into the HttpClient Factory as
the Primary handler. But here CachingHandler is a DelegatingHandler
and I didn't put hook inplace for that. Hence manually setting the
InnderHandler..
2. We're not leveraging the IoC container to activate
InMemoryCacheStore nor CachingHandler (with ICacheStore being injected
into it).
Need to think about this API a bit.
Example CacheCow test code here:
https://github.com/damianh/ProxyKit/blob/cachecow/src/ProxyKit.Tests/CacheCowTests.cs#L19
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#36 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABsWj9EZxpbdiizh_gZJjmZosEeCOWL2ks5vHEqZgaJpZM4aQ5DT>
.
|
I think it may be safer to hold own instance of InMemoryVaryHeaderStore as
well
…On Sat, 26 Jan 2019, 12:34 Jakub Konecki ***@***.*** wrote:
Just came up with the same myself ;-)
I don't really mind about not using DI for cache store - the whole proxy
thanks to your lib fits on one screen :-D
On Sat, 26 Jan 2019, 12:31 Damian Hickey ***@***.*** wrote:
> So this appears to work now:
>
> var cacheStore = new InMemoryCacheStore(TimeSpan.FromMinutes(1));services.AddProxy(options =>
> {
> options.GetMessageHandler = () => new CachingHandler(cacheStore)
> {
> InnerHandler = new HttpClientHandler {AllowAutoRedirect = false, UseCookies = false}
> };
> });
>
> Not quite pit-of-success...
>
> 1. options.GetMessageHandler is hooked into the HttpClient Factory as
> the Primary handler. But here CachingHandler is a DelegatingHandler
> and I didn't put hook inplace for that. Hence manually setting the
> InnderHandler..
> 2. We're not leveraging the IoC container to activate
> InMemoryCacheStore nor CachingHandler (with ICacheStore being
> injected into it).
>
> Need to think about this API a bit.
>
> Example CacheCow test code here:
> https://github.com/damianh/ProxyKit/blob/cachecow/src/ProxyKit.Tests/CacheCowTests.cs#L19
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#36 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABsWj9EZxpbdiizh_gZJjmZosEeCOWL2ks5vHEqZgaJpZM4aQ5DT>
> .
>
|
You do need to strongly manage the lifecycles of those, yes. |
Thank you very much, Damian! |
Production already? Lovely! 😄 Based on this discussion, I made some improvements to make this easier: #37 . This change was semver breaking so new major version (on nuget.org soon). Also added recipe for CacheCow and am supporting it via tests
I don't think an integration package is worth it right now but will keep open mind to it if there is more demand and makes things easier. Thanks for suggestion and the help! |
I think the recipe is a perfect solution - allows for full customisation
which would require a lot of work if CacheCow was hidden behind ProxyKit
façade.
…On Sun, 27 Jan 2019, 12:38 Damian Hickey ***@***.*** wrote:
Production already? Lovely! 😄
Based on this discussion, I made some improvements to make this easier:
#37 <#37> . This change was
semver breaking so new major version.
Also added recipe for CacheCow and am supporting it via tests
-
https://github.com/damianh/ProxyKit/blob/master/src/Recipes/11_CachingWithCacheCow.cs
-
https://github.com/damianh/ProxyKit/blob/master/src/ProxyKit.Tests/CacheCowTests.cs
I don't think an integration package is worth it right now but will keep
open mind to it if there is more demand and makes things easier.
Thanks for suggestion and the help!
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#36 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABsWj25WYeVCGtvl9DU7KhdaSxHpthiNks5vHZ3ZgaJpZM4aQ5DT>
.
|
I would like to cache responses received from upstream servers based on standard http caching headers.
My scenario involves some slow-changing reference data that could be cached by ProxyKit and save a network trip to the upstream server.
It would be nice to be able to enable caching for individual routes, but in my scenario that isn't necessary.
Happy to contribute when given some guidelines.
The text was updated successfully, but these errors were encountered: