ci: Add support for testing Python 3.12-3.14 #331
Conversation
|
Hi @iloveitaly, thanks for PR! If you don't mind, I'm going to add a couple more commits onto this to help get some other versioning changes finished. Then we'll be able to get your contribution shipped. |
nateprewitt
force-pushed
the
patch-1
branch
3 times, most recently
from
a55d929 to
73c5404
Compare
nateprewitt
changed the title
|
|
||
| steps: | ||
| - uses: actions/checkout@v2 | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
| - uses: actions/checkout@v4 | |
| - uses: actions/checkout@v5 |
jamesls
left a comment
jamesls
left a comment
There was a problem hiding this comment.
Is it possible to split out the testing related changes for new python versions from the caching issue? Or was there a specific reason we need this included with this PR?
jmespath/parser.py
Outdated
| def _free_cache_entries(self): | ||
| for key in random.sample(list(self._CACHE.keys()), int(self._MAX_SIZE / 2)): | ||
| self._CACHE.pop(key, None) | ||
| with self._CACHE_LOCK: |
There was a problem hiding this comment.
Remind me again, why do we need a lock here, what issue are addressing? Is it #334? I'd prefer addressing this without adding a lock.
There was a problem hiding this comment.
The issue is slightly different than #334 but the same underlying flaw. We sample our cache to do eviction which can already be in the process of being freed. If you run the existing test suite with Python 3.11+, you'll see test_thread_safety_of_cache fails for all versions.
We eagerly call _free_cache_entries every parse() invocation which puts this on the hot path. What makes it particularly noticeable though is our max cache size/eviction logic don't work how I think they were intended.
When we pop keys, we always remove a random sample of self._MAX_SIZE/2 (64) entries. The problem is we have no upper bound on the cache size, so it's possible to have cases where we have significantly more than 128 entries and a free may not bring us under the cache limit. That means multiple calls can trigger a free because we didn't do it properly the first attempt exacerbating the issue.
Happy to look at non-locking options but what would your high-level thought be there?
There was a problem hiding this comment.
I've opened #335 for more targeted discussion. I'll pull the cache changes out of this, we just won't be able to merge until the underlying issue is fixed in the test suite.
5 participants
No description provided.