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

fix(LRUCache): copy using deepcopy, and ensure the get_all function always terminates #3861

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

cmanallen
Copy link
Member

@cmanallen cmanallen commented Dec 5, 2024

@aliu39 discovered that under certain circumstances a process can get stuck in an infinite loop. Andrew fixed this by using deepcopy which prevents the infinite loop and fixes a bug where the LRU returns incorrect results. Additionally, I've added a terminating loop in case there are any future bugs we've missed.

Closes: #3862

Out of precaution, we disabled flagpole evaluation tracking Sentry while we wait for this to be merged.

Copy link

codecov bot commented Dec 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.93%. Comparing base (fd56608) to head (2d67b97).
Report is 1 commits behind head on master.

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3861      +/-   ##
==========================================
+ Coverage   79.91%   79.93%   +0.02%     
==========================================
  Files         137      137              
  Lines       15401    15403       +2     
  Branches     2619     2620       +1     
==========================================
+ Hits        12307    12312       +5     
+ Misses       2221     2219       -2     
+ Partials      873      872       -1     
Files with missing lines Coverage Δ
sentry_sdk/_lru_cache.py 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

@aliu39
Copy link
Member

aliu39 commented Dec 5, 2024

We found out the process will hang if you copy a LRUCache, then call get_all() on the original. It works as expected if you call get_all() on clones. Will add some coverage in FF tests for this case (fork a scope then read the flags on the original scope)

@aliu39 aliu39 changed the title fix: Ensure the get_all function always terminates fix(LRUCache): Use deepcopy and ensure the get_all function always terminates Dec 5, 2024
@aliu39 aliu39 changed the title fix(LRUCache): Use deepcopy and ensure the get_all function always terminates fix(LRUCache): copy using deepcopy, and ensure the get_all function always terminates Dec 5, 2024
@aliu39 aliu39 requested review from Zylphrex and a team December 5, 2024 23:39
@antonpirker antonpirker merged commit 8f9461e into master Dec 6, 2024
136 checks passed
@antonpirker antonpirker deleted the cmanallen/fix-deadlock branch December 6, 2024 08:11
@antonpirker
Copy link
Member

This has been released with version 2.19.2: https://github.com/getsentry/sentry-python/blob/master/CHANGELOG.md#2192

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

Successfully merging this pull request may close these issues.

Infinite loop when serializing feature flags
3 participants