Uh oh!
There was an error while loading. Please reload this page.
fix(ssl): add lru_cache for create_ssl_context - #3053
Conversation
f500e0e to
339a05dCompareT-256
commented
Jan 14, 2024
Thanks for the PR, I like the solution. This is high memory usage I encountered when using tons of Client instance in an app (I need them for several reasons). But perhaps you would like to see and wait for #3022 @karpetrosyan could new |
karpetrosyan
commented
Jan 15, 2024
Are we really capable of doing something like this? What if the SSL context is modified? Why should any subsequent function call return the modified SSL context? I mean... importsslfromfunctoolsimportcache@cachedefget_ssl_context():
returnssl.create_default_context()
first_ssl=get_ssl_context()
first_ssl.check_hostname=Falsesecond_ssl=get_ssl_context()
print(f.check_hostname) # False |
shepilov-vladislav
commented
Jan 15, 2024
@T-256, @karpetrosyan First of all thank you for your quick response and help. The thing is that we make a lot of requests using http and we tried to replace After adding caching - CPU consumption dropped by more than 4x on our Pods. If there is another more correct way to do it - please advise. I haven't found a way to do it without changing the Perhaps this PR will provide that opportunity, but I'm not sure If you still find my changes useful, just tell me, I'll fix the tests and we'll merge this one out ✌️ |
karpetrosyan
commented
Jan 15, 2024
We should probably wait until #3022 is marged before we consider adding cache at that layer. I like this idea, because configuring SSL context is a very expensive job. |
lovelydinosaur
commented
Jan 15, 2024
Prob indicates that you're instantiating clients repeatedly, and losing out on the benefits of connection pooling.
Would be simpler to consider then yep. (Anyone want to open a "SSL Cache" discussion?) |
T-256
commented
Sep 23, 2024
I don't think we would need that. by refer to this comment, it'd be do the same job of caching to use single global variable cross clients. |
Summary
Checklist