-
Notifications
You must be signed in to change notification settings - Fork 517
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 lru cache copying #3883
Fix lru cache copying #3883
Conversation
Walk the circularly linked list instead of using (deep-)copy on its attributes
There was a problem hiding this 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.
There was a problem hiding this 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!
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
|
There was a problem hiding this 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.
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