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

Feature: Add caching for HTTP responses #62

Merged
merged 5 commits into from
Dec 14, 2023

Conversation

karpetrosyan
Copy link
Contributor

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 yanyongyu added the enhancement New feature or request label Dec 8, 2023
@yanyongyu
Copy link
Owner

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
Copy link
Contributor Author

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.

githubkit/core.py Outdated Show resolved Hide resolved
@yanyongyu yanyongyu changed the title Add caching for HTTP responses Feature: Add caching for HTTP responses Dec 14, 2023
@yanyongyu yanyongyu merged commit d005552 into yanyongyu:master Dec 14, 2023
2 checks passed
@yanyongyu
Copy link
Owner

yanyongyu commented Dec 29, 2023

@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
Copy link
Contributor Author

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

@karpetrosyan
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use HTTP caching
2 participants