Skip to content

Conversation

ffelixg
Copy link
Contributor

@ffelixg ffelixg commented Dec 18, 2024

This addresses the lru cache copy function by walking the circularly linked list and creating new cache/root attributes instead of using (deep-)copy on the source.

Fixes #3852

ffelixg and others added 4 commits December 19, 2024 00:14
Copy link
Member

@antonpirker antonpirker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Because it is some tricky code, I will ask for some other engineers to review it before I will merge it.

Copy link
Member

@cmanallen cmanallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ffelixg Thanks for your interest in fixing this bug! I left some review comments. You can also review my PR here to see how I implemented it: https://github.com/getsentry/sentry-python/pull/3882/files. Obviously I'm biased in favor of my solution to the problem but you're welcome to let me know where I've made short comings!

@antonpirker antonpirker added the Trigger: tests using secrets PR code is safe; run CI label Dec 20, 2024
Copy link

codecov bot commented Dec 20, 2024

Codecov Report

Attention: Patch coverage is 93.10345% with 2 lines in your changes missing coverage. Please review.

Project coverage is 79.89%. Comparing base (8ced660) to head (f98f6a9).
Report is 1 commits behind head on master.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
sentry_sdk/_lru_cache.py 93.10% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3883      +/-   ##
==========================================
- Coverage   79.91%   79.89%   -0.02%     
==========================================
  Files         139      139              
  Lines       15458    15429      -29     
  Branches     2625     2624       -1     
==========================================
- Hits        12353    12327      -26     
+ Misses       2232     2228       -4     
- Partials      873      874       +1     
Files with missing lines Coverage Δ
sentry_sdk/_lru_cache.py 94.87% <93.10%> (-5.13%) ⬇️

... and 3 files with indirect coverage changes

Copy link
Member

@antonpirker antonpirker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a quick discussion here #3852 we will settle on a simpler lru cache implementation.

This looks fine now and has all the tests, so it is good to be merged.

@antonpirker antonpirker dismissed cmanallen’s stale review December 20, 2024 11:31

Review was for an old implementation.

@antonpirker antonpirker merged commit f6281f5 into getsentry:master Dec 20, 2024
138 of 140 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Trigger: tests using secrets PR code is safe; run CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LRUCache.__copy__ is unsound
3 participants