search

yanyongyu / githubkit

The modern, all-batteries-included GitHub SDK for Python, including rest api, graphql, webhooks, like octokit!
https://yanyongyu.github.io/githubkit/
MIT License
177 stars 25 forks source link

Feature: Add caching for HTTP responses #62

Closed karpetrosyan closed 9 months ago

karpetrosyan commented 10 months ago

Closes #61

Adds RFC9111 HTTP caching, which caches responses that the server suggests reusing.

If we want to provide a way to disable caching, perhaps we should add something like http_cache boolean to the GitHub class?

yanyongyu commented 10 months ago

We can add a http_cache option to the GitHub class and user may set it as None or False to disable, True for default in-mem or some storage provider settings to control the caching behavior?

karpetrosyan commented 9 months ago

some storage provider settings to control the caching behavior?

We could inject httpx.Transport or event httpx.Client instances, but I believe that for now, this simple minimal change will suffice.

yanyongyu commented 9 months ago

@karpetrosyan hello, i have encountered some errors when testing, it seems is caused by this pr.

The transport socket is not closed after httpx client closed. The GC throws the error when collect the transport. Take a look at the code and find that the httpx.HTTPTransport() is not closed. wrapped transport only close the storage.

https://github.com/karpetrosyan/hishel/blob/0948f9e59e123f2703e199b844fcc394696b5921/hishel/_async/_transports.py#L235-L236 https://github.com/karpetrosyan/hishel/blob/0948f9e59e123f2703e199b844fcc394696b5921/hishel/_sync/_transports.py#L235-L236

I don't know if this is as designed for hishel? githubkit may needs to close the inner transport at the same time.

karpetrosyan commented 9 months ago

Hi! Yes, I see the issue. I'm going to release hishel today with this problem fixed.

karpetrosyan commented 9 months ago

Hishel v0.0.21 is out https://pypi.org/project/hishel/! Thanks for contribution