Skip to content

fix: Deepcopy flag buffer on scope fork #3882

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

Closed
wants to merge 9 commits into from

Conversation

cmanallen
Copy link
Member

@cmanallen cmanallen commented Dec 18, 2024

A previous attempt to fix this issue focused on a related issue and assumed that it would fix this. This PR adds a test using the reproduction steps mentioned in the bug report and fixes the issue by deep copying the buffer.

Closes: #3852

Copy link

codecov bot commented Dec 18, 2024

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
13814 1 13813 4139
View the top 1 failed tests by shortest run time
tests.integrations.stdlib.test_httplib test_http_timeout
Stack Traces | 75.2s run time
.../integrations/stdlib/test_httplib.py:373: in test_http_timeout
    assert len(transaction["spans"]) == 1
E   assert 0 == 1
E    +  where 0 = len([])

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

@cmanallen cmanallen enabled auto-merge (squash) December 18, 2024 21:36
@cmanallen
Copy link
Member Author

@antonpirker Re-ran the test and this is now passing. Has test_http_timeout always been flakey? Maybe it should be skipped?

@@ -91,13 +89,6 @@ def __init__(self, max_size):

self.hits = self.misses = 0

def __copy__(self):
Copy link
Contributor

@ffelixg ffelixg Dec 19, 2024

Choose a reason for hiding this comment

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

If you remove this method entirely, aren't we back at square one? I.e. the cache will get polluted by changes to a copy:

def test_mutate():
    cache = LRUCache(1)
    cache.set(0, 0)
    copy(cache).set(0, 1)
    assert cache.get(0) == 0 # fails

Copy link
Member Author

@cmanallen cmanallen Dec 19, 2024

Choose a reason for hiding this comment

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

No because the call site is now using deepcopy. Which targets the dunder deepcopy method which we leave as default.

@antonpirker
Copy link
Member

Closing this in favor of solution linked here: #3852

auto-merge was automatically disabled December 20, 2024 11:34

Pull request was closed

@antonpirker antonpirker deleted the cmanallen/deepcopy-root branch April 22, 2025 09:28
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.

LRUCache.__copy__ is unsound
3 participants