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.

Sign up for GitHub

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
Closed

fix: Deepcopy flag buffer on scope fork #3882

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
08796fa
Deepcopy the cache
cmanallen Dec 18, 2024
80fccd6
Remove copy dunder method
cmanallen Dec 18, 2024
7edca64
Add coverage for the flag buffer on scope fork
cmanallen Dec 18, 2024
fc33085
Add mutation event after fork to the old scope
cmanallen Dec 18, 2024
dfa9225
Add assertions
cmanallen Dec 18, 2024
a780388
Assert the scopes can mutate without interfering with one another
cmanallen Dec 18, 2024
2f22809
Add deepcopy coverage
cmanallen Dec 19, 2024
71dd591
Remove extraneous line break
cmanallen Dec 19, 2024
832294a
Merge branch 'master' into cmanallen/deepcopy-root
antonpirker Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions sentry_sdk/_lru_cache.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,6 @@

"""

from copy import copy, deepcopy

SENTINEL = object()


Expand Down Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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.

cache = LRUCache(self.max_size)
cache.full = self.full
cache.cache = copy(self.cache)
cache.root = deepcopy(self.root)
return cache

def set(self, key, value):
link = self.cache.get(key, SENTINEL)

Expand Down
7 changes: 0 additions & 7 deletions sentry_sdk/flag_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
from copy import copy
from typing import TYPE_CHECKING

import sentry_sdk
Expand All @@ -25,12 +24,6 @@ def clear(self):
# type: () -> None
self.buffer = LRUCache(self.capacity)

def __copy__(self):
# type: () -> FlagBuffer
buffer = FlagBuffer(capacity=self.capacity)
buffer.buffer = copy(self.buffer)
return buffer

def get(self):
# type: () -> list[FlagData]
return [{"flag": key, "result": value} for key, value in self.buffer.get_all()]
Expand Down
4 changes: 2 additions & 2 deletions sentry_sdk/scope.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import os
import sys
import warnings
from copy import copy
from copy import copy, deepcopy
from collections import deque
from contextlib import contextmanager
from enum import Enum
Expand Down Expand Up @@ -252,7 +252,7 @@ def __copy__(self):

rv._last_event_id = self._last_event_id

rv._flags = copy(self._flags)
rv._flags = deepcopy(self._flags)

return rv

Expand Down
15 changes: 12 additions & 3 deletions tests/test_lru_cache.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import pytest
from copy import copy
from copy import deepcopy

from sentry_sdk._lru_cache import LRUCache

Expand Down Expand Up @@ -66,13 +66,22 @@ def test_cache_copy():
cache.set(0, 0)
cache.set(1, 1)

copied = copy(cache)
copied = deepcopy(cache)
cache.set(2, 2)
cache.set(3, 3)
assert copied.get_all() == [(0, 0), (1, 1)]
assert cache.get_all() == [(1, 1), (2, 2), (3, 3)]

copied = copy(cache)
copied = deepcopy(cache)
cache.get(1)
assert copied.get_all() == [(1, 1), (2, 2), (3, 3)]
assert cache.get_all() == [(2, 2), (3, 3), (1, 1)]


def test_cache_pollution():
cache1 = LRUCache(max_size=2)
cache1.set(1, True)
cache2 = deepcopy(cache1)
cache2.set(1, False)
assert cache1.get(1) is True
assert cache2.get(1) is False
22 changes: 22 additions & 0 deletions tests/test_scope.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,28 @@ def test_all_slots_copied():
assert getattr(scope_copy, attr) == getattr(scope, attr)


def test_scope_flags_copy():
# Assert forking creates a deepcopy of the flag buffer. The new
# scope is free to mutate without consequence to the old scope. The
# old scope is free to mutate without consequence to the new scope.
old_scope = Scope()
old_scope.flags.set("a", True)

new_scope = old_scope.fork()
new_scope.flags.set("a", False)
old_scope.flags.set("b", True)
new_scope.flags.set("c", True)

assert old_scope.flags.get() == [
{"flag": "a", "result": True},
{"flag": "b", "result": True},
]
assert new_scope.flags.get() == [
{"flag": "a", "result": False},
{"flag": "c", "result": True},
]


def test_merging(sentry_init, capture_events):
sentry_init()

Expand Down
Loading