Fix lru cache copying#3883
Conversation
Walk the circularly linked list instead of using (deep-)copy on its attributes
antonpirker
left a comment
There was a problem hiding this comment.
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.
cmanallen
left a comment
There was a problem hiding this comment.
@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!
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
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
|
antonpirker
left a comment
There was a problem hiding this comment.
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.
Review was for an old implementation.
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