Repository navigation
Make thread-safe - #1346
Make thread-safe#1346
Conversation
Codecov Reportβ
All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1346 +/- ##
==========================================
+ Coverage 97.10% 97.18% +0.07%
==========================================
Files 236 236
Lines 3008 3053 +45
==========================================
+ Hits 2921 2967 +46
+ Misses 87 86 -1 β View full report in Codecov by Harness. π New features to boost your workflow:
|
|
Thanks for this. I verified the branch empirically rather than just reading it, and the locking holds up. My verdict: this is mergeable β approve with comments. Nothing I found is a regression introduced by this PR, but two of the "Fixes #X" claims don't hold up, one of the four new tests doesn't test anything, and there's a second non-race bug in the same function that keeps #1291 partly open. Setup: What I confirmed worksThe races are real and this fixes them. To isolate the race from the logic bug discussed below, I made every settings object use the same
Both master failure modes disappear: the check-then-read TOCTOU in the No deadlock. Since the lock is reentrant and taken on nested calls, I checked for it explicitly with a watchdog that samples a progress counter and dumps every thread's stack via Realistic mixed usage is clean. Heterogeneous stress across four entry points ( No regressions, and it merges cleanly. Full suite: master 24158 passed / 0 failed, this branch 24055 passed / 0 failed (all skips are environment noise β missing Three of the four new tests are genuine regression tests. Running
I also checked the hunks that aren't pure locking: the inverted early return in 1. A second, non-race bug in the same function keeps #1291 partly open
# dictionary.py L190-197
cache.setdefault(self._settings.registry_key, {})[self.info["name"]] = value
if self._settings.CACHE_SIZE_LIMIT and len(cache) > self._settings.CACHE_SIZE_LIMIT:
cache.pop(list(cache.keys())[0]) # oldest key β which may BE registry_keyThe five caches are class-level and shared by every Three lines, no threads, public API only: import dateparser
dateparser.parse("1 day ago", languages=["en"], settings={"CACHE_SIZE_LIMIT": 1})
dateparser.parse("1 day ago", languages=["en"], settings={"CACHE_SIZE_LIMIT": 1000})
dateparser.parse("il y a 1 jour", languages=["fr"], settings={"CACHE_SIZE_LIMIT": 1}) # raisesSame terminal frame as the traceback in #1291. It raises identically on master ( Scaled up (
The single-threaded numbers are identical because this path is untouched; the 59% β 42% drop under threads is the race component being removed, and the residual is this logic bug leaking through. All five caches are affected β at 48 threads Γ 20 000 calls I get To pin down when it can fire, I model-checked the eviction rule over random operation sequences: uniform Suggested fix β re-insert the just-written key so it becomes the newest; then def _add_to_cache(self, value, cache):
with self._cache_lock:
key = self._settings.registry_key
# Re-insert so the entry just written is the newest. dicts keep
# insertion order, so the oldest key can then never be this one --
# which matters because callers may disagree about CACHE_SIZE_LIMIT,
# so len(cache) can already exceed *this* caller's limit while its
# own key is the oldest, and the caller reads that entry back
# immediately.
entry = cache.pop(key, {})
entry[self.info["name"]] = value
cache[key] = entry
limit = self._settings.CACHE_SIZE_LIMIT
if limit and len(cache) > limit:
cache.pop(next(iter(cache)))Applied on top of your branch this takes every reproduction above to 0 errors (including 16 threads Γ 2000, previously 846), keeps the existing suite green (24055 passed / 0 failed, unchanged), and your four thread-safety tests still pass. One note if you'd rather drain to the limit in a loop instead: don't. I measured that variant β with a burst of 1000 adds from a big-limit caller followed by one add from a 2.
|
|
Post-merge follow-up on my review above. 1.
Both causes are gone: the check-then-read race and the self-eviction. One detail I hadn't isolated before β on pre-merge master, 10 of those 12 2. Cutting the iteration counts did not cost coverage. This was the one change I wanted to verify, because 40β24 dictionaries / 300β50 inner iterations / 3000β100 parses / 150β40 searches is a 10β30Γ reduction, and a regression test that can no longer fail is worse than a slow one. They still fail. Merged
And the CI cost is essentially gone. Full 3. One gap: nothing locks in the The natural regression test is single-threaded, needs no locks and costs milliseconds: def test_add_to_cache_does_not_evict_the_entry_it_just_wrote(self):
# Issue #1291: the caches are shared but CACHE_SIZE_LIMIT comes from the
# calling settings, so len(cache) can already exceed *this* caller's limit
# while its own key is the oldest. Evicting the oldest key would then drop
# the entry the caller reads back on the next line.
dateparser.parse("1 day ago", languages=["en"], settings={"CACHE_SIZE_LIMIT": 1})
dateparser.parse("1 day ago", languages=["en"], settings={"CACHE_SIZE_LIMIT": 1000})
self.assertIsNotNone(
dateparser.parse("il y a 1 jour", languages=["fr"], settings={"CACHE_SIZE_LIMIT": 1})
)That raises 4. The
Nothing here needs anything from this PR β it merged in good shape. |
Fixes #441, fixes #1291.