diff --git a/sentry_sdk/client.py b/sentry_sdk/client.py index 7692427fa3..2cbc8d6398 100644 --- a/sentry_sdk/client.py +++ b/sentry_sdk/client.py @@ -24,7 +24,6 @@ ) from sentry_sdk.envelope import Envelope, Item from sentry_sdk.integrations import setup_integrations -from sentry_sdk.integrations.dedupe import DedupeIntegration from sentry_sdk.monitor import Monitor from sentry_sdk.profiler.continuous_profiler import setup_continuous_profiler from sentry_sdk.serializer import serialize @@ -594,13 +593,6 @@ def _prepare_event( ) self.transport.record_lost_event(reason, data_category="error") - # If this is an exception, reset the DedupeIntegration. It still - # remembers the dropped exception as the last exception, meaning - # that if the same exception happens again and is not dropped - # in before_send, it'd get dropped by DedupeIntegration. - if event.get("exception"): - DedupeIntegration.reset_last_seen() - event = new_event return event diff --git a/sentry_sdk/integrations/dedupe.py b/sentry_sdk/integrations/dedupe.py index a0cc88081f..a79b05bd29 100644 --- a/sentry_sdk/integrations/dedupe.py +++ b/sentry_sdk/integrations/dedupe.py @@ -1,14 +1,12 @@ -import weakref -from contextvars import ContextVar from typing import TYPE_CHECKING import sentry_sdk from sentry_sdk.integrations import Integration from sentry_sdk.scope import add_global_event_processor -from sentry_sdk.utils import logger +from sentry_sdk.utils import capture_internal_exceptions, logger if TYPE_CHECKING: - from typing import Any, Optional + from typing import Optional from sentry_sdk._types import Event, Hint @@ -16,9 +14,6 @@ class DedupeIntegration(Integration): identifier = "dedupe" - def __init__(self) -> None: - self._last_seen: "ContextVar[Any]" = ContextVar("last-seen") - @staticmethod def setup_once() -> None: @add_global_event_processor @@ -34,30 +29,12 @@ def processor(event: "Event", hint: "Optional[Hint]") -> "Optional[Event]": if exc_info is None: return event - last_seen = integration._last_seen.get(None) - if last_seen is not None: - # last_seen is either a weakref or the original instance - last_seen = ( - last_seen() if isinstance(last_seen, weakref.ref) else last_seen - ) - exc = exc_info[1] - if last_seen is exc: + + if getattr(exc, "_handled_by_sentry", False): logger.info("DedupeIntegration dropped duplicated error event %s", exc) return None - - # we can only weakref non builtin types - try: - integration._last_seen.set(weakref.ref(exc)) - except TypeError: - integration._last_seen.set(exc) - - return event - - @staticmethod - def reset_last_seen() -> None: - integration = sentry_sdk.get_client().get_integration(DedupeIntegration) - if integration is None: - return - - integration._last_seen.set(None) + else: + with capture_internal_exceptions(): + exc._handled_by_sentry = True + return event diff --git a/tests/test_basics.py b/tests/test_basics.py index 38861c4cf9..ce640b4c9e 100644 --- a/tests/test_basics.py +++ b/tests/test_basics.py @@ -1,9 +1,11 @@ import datetime +import gc import importlib import logging import os import sys import time +import weakref from collections import Counter import pytest @@ -27,6 +29,7 @@ Integration, setup_integrations, ) +from sentry_sdk.integrations.dedupe import DedupeIntegration from sentry_sdk.integrations.logging import LoggingIntegration from sentry_sdk.integrations.stdlib import StdlibIntegration from sentry_sdk.scope import add_global_event_processor @@ -611,19 +614,75 @@ def before_send(event, hint): sentry_init(before_send=before_send) events = capture_events() - exc = ValueError("aha!") for _ in range(2): # The first ValueError will be dropped by before_send. The second # ValueError will be accepted by before_send, and should be sent to # Sentry. try: - raise exc + raise ValueError("aha!") + except Exception: + capture_exception() + + assert len(events) == 1 + + +def test_dedupe_drops_exception_when_seen_a_second_time(sentry_init, capture_events): + """ + This test is intended to emulate behavior seen in frameworks like Django, + where an exception is raised in a view and then is re-raised in middleware. + + In cases like that we don't want to send a second event for that exception. + """ + sentry_init() + events = capture_events() + + test = None + for _ in range(2): + try: + if test is None: + test = ValueError("foo") + raise test except Exception: capture_exception() assert len(events) == 1 +def test_dedupe_does_not_retain_builtin_exceptions(sentry_init): + """ + There was a different approach that used to be used by DedupeIntegration + that used a weakref to hold a reference to a seen exception, and then do a comparison + on an incoming exception with that weakref to determine if it was a duplicate. + + Built in exceptions such as ValueError couldn't be used with weakref, so we would instead + hold a strong reference to that exception. However, this led to memory leaks as described in + https://github.com/getsentry/sentry-python/issues/6094 + """ + sentry_init(default_integrations=False, integrations=[DedupeIntegration()]) + + class Payload: + pass + + payload_ref = None + + def fail(): + nonlocal payload_ref + payload = Payload() + payload_ref = weakref.ref(payload) + raise ValueError("boom") + + def capture(): + try: + fail() + except ValueError as e: + sentry_sdk.capture_exception(e) + + capture() + + gc.collect() + assert payload_ref() is None + + def test_event_processor_drop_records_client_report( sentry_init, capture_events, capture_record_lost_event_calls ):