From 08796fa91c4ad00d662b8834e910b434090a0dc5 Mon Sep 17 00:00:00 2001 From: Colton Allen Date: Wed, 18 Dec 2024 14:28:44 -0600 Subject: [PATCH 1/8] Deepcopy the cache --- sentry_sdk/_lru_cache.py | 4 ++-- tests/test_lru_cache.py | 9 +++++++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/sentry_sdk/_lru_cache.py b/sentry_sdk/_lru_cache.py index 825c773529..9b46d304e3 100644 --- a/sentry_sdk/_lru_cache.py +++ b/sentry_sdk/_lru_cache.py @@ -62,7 +62,7 @@ """ -from copy import copy, deepcopy +from copy import deepcopy SENTINEL = object() @@ -94,7 +94,7 @@ def __init__(self, max_size): def __copy__(self): cache = LRUCache(self.max_size) cache.full = self.full - cache.cache = copy(self.cache) + cache.cache = deepcopy(self.cache) cache.root = deepcopy(self.root) return cache diff --git a/tests/test_lru_cache.py b/tests/test_lru_cache.py index cab9bbc7eb..ed5d673a3b 100644 --- a/tests/test_lru_cache.py +++ b/tests/test_lru_cache.py @@ -76,3 +76,12 @@ def test_cache_copy(): 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_no_overwrites_to_parent(): + cache1 = LRUCache(max_size=2) + cache1.set(1, True) + cache2 = cache1.__copy__() + cache2.set(1, False) + assert cache1.get(1) is True + assert cache2.get(1) is False From 80fccd6aa2166e4be41558b00962b067a17c1d86 Mon Sep 17 00:00:00 2001 From: Colton Allen Date: Wed, 18 Dec 2024 14:42:52 -0600 Subject: [PATCH 2/8] Remove copy dunder method --- sentry_sdk/_lru_cache.py | 9 --------- sentry_sdk/flag_utils.py | 7 ------- 2 files changed, 16 deletions(-) diff --git a/sentry_sdk/_lru_cache.py b/sentry_sdk/_lru_cache.py index 9b46d304e3..36b7c5e571 100644 --- a/sentry_sdk/_lru_cache.py +++ b/sentry_sdk/_lru_cache.py @@ -62,8 +62,6 @@ """ -from copy import deepcopy - SENTINEL = object() @@ -91,13 +89,6 @@ def __init__(self, max_size): self.hits = self.misses = 0 - def __copy__(self): - cache = LRUCache(self.max_size) - cache.full = self.full - cache.cache = deepcopy(self.cache) - cache.root = deepcopy(self.root) - return cache - def set(self, key, value): link = self.cache.get(key, SENTINEL) diff --git a/sentry_sdk/flag_utils.py b/sentry_sdk/flag_utils.py index 2b345a7f0b..cf4800e855 100644 --- a/sentry_sdk/flag_utils.py +++ b/sentry_sdk/flag_utils.py @@ -1,4 +1,3 @@ -from copy import copy from typing import TYPE_CHECKING import sentry_sdk @@ -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()] From 7edca64210f2ff584738cd3c7f8c945d754d9922 Mon Sep 17 00:00:00 2001 From: Colton Allen Date: Wed, 18 Dec 2024 14:44:03 -0600 Subject: [PATCH 3/8] Add coverage for the flag buffer on scope fork --- sentry_sdk/scope.py | 4 ++-- tests/test_lru_cache.py | 27 --------------------------- tests/test_scope.py | 16 ++++++++++++++++ 3 files changed, 18 insertions(+), 29 deletions(-) diff --git a/sentry_sdk/scope.py b/sentry_sdk/scope.py index bb45143c48..cf72fabdd1 100644 --- a/sentry_sdk/scope.py +++ b/sentry_sdk/scope.py @@ -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 @@ -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 diff --git a/tests/test_lru_cache.py b/tests/test_lru_cache.py index ed5d673a3b..3e9c0ac964 100644 --- a/tests/test_lru_cache.py +++ b/tests/test_lru_cache.py @@ -1,5 +1,4 @@ import pytest -from copy import copy from sentry_sdk._lru_cache import LRUCache @@ -59,29 +58,3 @@ def test_cache_get_all(): assert cache.get_all() == [(1, 1), (2, 2), (3, 3)] cache.get(1) assert cache.get_all() == [(2, 2), (3, 3), (1, 1)] - - -def test_cache_copy(): - cache = LRUCache(3) - cache.set(0, 0) - cache.set(1, 1) - - copied = copy(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) - 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_no_overwrites_to_parent(): - cache1 = LRUCache(max_size=2) - cache1.set(1, True) - cache2 = cache1.__copy__() - cache2.set(1, False) - assert cache1.get(1) is True - assert cache2.get(1) is False diff --git a/tests/test_scope.py b/tests/test_scope.py index a03eb07a99..8a2f23a504 100644 --- a/tests/test_scope.py +++ b/tests/test_scope.py @@ -43,6 +43,22 @@ def test_all_slots_copied(): assert getattr(scope_copy, attr) == getattr(scope, attr) +def test_scope_flags_copy(): + # A scope exists and a flag is appended to it. + old_scope = Scope() + old_scope.flags.set("a", True) + + # Scope is forked; flag buffer is copied and contains the data of + # the parent. The flag is overwritten. + new_scope = old_scope.fork() + new_scope.flags.set("a", False) + + # New scope has the change. Old scope was not polluted by changes + # to the new scope. + old_scope.flags.get() == [{"flag": "a", "result": True}] + new_scope.flags.get() == [{"flag": "a", "result": False}] + + def test_merging(sentry_init, capture_events): sentry_init() From fc33085c023d18a5426d3dadde556e063f1cc491 Mon Sep 17 00:00:00 2001 From: Colton Allen Date: Wed, 18 Dec 2024 14:48:31 -0600 Subject: [PATCH 4/8] Add mutation event after fork to the old scope --- tests/test_scope.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tests/test_scope.py b/tests/test_scope.py index 8a2f23a504..6118564381 100644 --- a/tests/test_scope.py +++ b/tests/test_scope.py @@ -49,13 +49,18 @@ def test_scope_flags_copy(): old_scope.flags.set("a", True) # Scope is forked; flag buffer is copied and contains the data of - # the parent. The flag is overwritten. + # the parent. The flag is overwritten on the child. The parent + # scope can continue mutating without interfering with the child. new_scope = old_scope.fork() new_scope.flags.set("a", False) + old_scope.flags.set("b", True) # New scope has the change. Old scope was not polluted by changes # to the new scope. - old_scope.flags.get() == [{"flag": "a", "result": True}] + old_scope.flags.get() == [ + {"flag": "a", "result": True}, + {"flag": "b", "result": True}, + ] new_scope.flags.get() == [{"flag": "a", "result": False}] From dfa9225597d24cc0cb92c28c8f3167583f52309d Mon Sep 17 00:00:00 2001 From: Colton Allen Date: Wed, 18 Dec 2024 14:54:22 -0600 Subject: [PATCH 5/8] Add assertions --- tests/test_scope.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_scope.py b/tests/test_scope.py index 6118564381..304f5d0eaf 100644 --- a/tests/test_scope.py +++ b/tests/test_scope.py @@ -57,11 +57,11 @@ def test_scope_flags_copy(): # New scope has the change. Old scope was not polluted by changes # to the new scope. - old_scope.flags.get() == [ + assert old_scope.flags.get() == [ {"flag": "a", "result": True}, {"flag": "b", "result": True}, ] - new_scope.flags.get() == [{"flag": "a", "result": False}] + assert new_scope.flags.get() == [{"flag": "a", "result": False}] def test_merging(sentry_init, capture_events): From a780388501a7e55d426ed3b8f314ac4ed4d8f754 Mon Sep 17 00:00:00 2001 From: Colton Allen Date: Wed, 18 Dec 2024 17:35:07 -0600 Subject: [PATCH 6/8] Assert the scopes can mutate without interfering with one another --- tests/test_scope.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/tests/test_scope.py b/tests/test_scope.py index 304f5d0eaf..9b16dc4344 100644 --- a/tests/test_scope.py +++ b/tests/test_scope.py @@ -44,24 +44,25 @@ def test_all_slots_copied(): def test_scope_flags_copy(): - # A scope exists and a flag is appended to it. + # 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) - # Scope is forked; flag buffer is copied and contains the data of - # the parent. The flag is overwritten on the child. The parent - # scope can continue mutating without interfering with the child. new_scope = old_scope.fork() new_scope.flags.set("a", False) old_scope.flags.set("b", True) + new_scope.flags.set("c", True) - # New scope has the change. Old scope was not polluted by changes - # to the new scope. assert old_scope.flags.get() == [ {"flag": "a", "result": True}, {"flag": "b", "result": True}, ] - assert new_scope.flags.get() == [{"flag": "a", "result": False}] + assert new_scope.flags.get() == [ + {"flag": "a", "result": False}, + {"flag": "c", "result": True}, + ] def test_merging(sentry_init, capture_events): From 2f22809817b893526e8edee8a63ce6dc4c974b19 Mon Sep 17 00:00:00 2001 From: Colton Allen Date: Wed, 18 Dec 2024 18:24:22 -0600 Subject: [PATCH 7/8] Add deepcopy coverage --- tests/test_lru_cache.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/test_lru_cache.py b/tests/test_lru_cache.py index 3e9c0ac964..d478ae7c99 100644 --- a/tests/test_lru_cache.py +++ b/tests/test_lru_cache.py @@ -1,5 +1,7 @@ import pytest +from copy import deepcopy + from sentry_sdk._lru_cache import LRUCache @@ -58,3 +60,29 @@ def test_cache_get_all(): assert cache.get_all() == [(1, 1), (2, 2), (3, 3)] cache.get(1) assert cache.get_all() == [(2, 2), (3, 3), (1, 1)] + + +def test_cache_copy(): + cache = LRUCache(3) + cache.set(0, 0) + cache.set(1, 1) + + 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 = 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 From 71dd59112f442101156f1812a171daf6db7eab28 Mon Sep 17 00:00:00 2001 From: Colton Allen Date: Wed, 18 Dec 2024 18:25:13 -0600 Subject: [PATCH 8/8] Remove extraneous line break --- tests/test_lru_cache.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/test_lru_cache.py b/tests/test_lru_cache.py index d478ae7c99..615c66f4a0 100644 --- a/tests/test_lru_cache.py +++ b/tests/test_lru_cache.py @@ -1,5 +1,4 @@ import pytest - from copy import deepcopy from sentry_sdk._lru_cache import LRUCache