search

requests-cache / aiohttp-client-cache

An async persistent cache for aiohttp requests
MIT License
118 stars 20 forks source link

Add asyncio.Lock mutex to _request #227

Closed rudcode closed 7 months ago

rudcode commented 7 months ago

With this change if there is simultaneous request to the same url at the same time, it will only fetch the url once and the rest is feeded from cache

@olk-m @JWCook

I create a test like this, is it enough?

codecov[bot] commented 7 months ago

Codecov Report

Attention: Patch coverage is 88.88889% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 97.31%. Comparing base (5b54454) to head (af930dc). Report is 1 commits behind head on main.

Files Patch % Lines
aiohttp_client_cache/session.py 87.80% 4 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #227 +/- ## ========================================== - Coverage 97.75% 97.31% -0.44% ========================================== Files 10 10 Lines 1025 1044 +19 Branches 173 177 +4 ========================================== + Hits 1002 1016 +14 - Misses 16 20 +4 - Partials 7 8 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

JWCook commented 7 months ago

Another thing to consider is lock cleanup. For a long-running cache with a large number of unique requests, the number of cache keys in memory could start to add up (roughly 1MB per 6K unique requests). I don't think that needs to be solved in this PR, though. I'll create a separate issue for that (#228).

rudcode commented 7 months ago

Hi @JWCook, I use the lru_cache like you mention in #228, is it okay if I implement like this af930dc? Is 16k enough for the maxsize?

@JWCook @olk-m I use if/else for the python version but I think this makes the build check failed. What do you think?

JWCook commented 7 months ago

I use the lru_cache like you mention in #228, is it okay if I implement like this https://github.com/requests-cache/aiohttp-client-cache/commit/af930dc482bfe00ed21c34d849090770a4ec02fa? Is 16k enough for the maxsize?

Yes, that looks good to me!

I use if/else for the python version but I think this makes the build check failed

That's fine, it looks like that's just because test coverage is only run for python 3.11 right now, so it thinks that else branch (python <=3.9) isn't covered.

JWCook commented 7 months ago

Merged. Thanks for the contribution @rudcode!