diff --git a/.changelog/5329.changed b/.changelog/5329.changed new file mode 100644 index 00000000000..84bb51d828b --- /dev/null +++ b/.changelog/5329.changed @@ -0,0 +1 @@ +opentelemetry-sdk: revert BoundedAttributes RLock back to Lock \ No newline at end of file diff --git a/opentelemetry-api/src/opentelemetry/attributes/__init__.py b/opentelemetry-api/src/opentelemetry/attributes/__init__.py index 15519ce0c2c..9c1b83b91e6 100644 --- a/opentelemetry-api/src/opentelemetry/attributes/__init__.py +++ b/opentelemetry-api/src/opentelemetry/attributes/__init__.py @@ -262,7 +262,7 @@ def __init__( MutableMapping[str, types.AnyValue] | OrderedDict[str, types.AnyValue] ) = {} - self._lock = threading.RLock() + self._lock = threading.Lock() if attributes: for key, value in attributes.items(): self[key] = value diff --git a/opentelemetry-api/tests/attributes/test_attributes.py b/opentelemetry-api/tests/attributes/test_attributes.py index fb6259f8c47..282300256fd 100644 --- a/opentelemetry-api/tests/attributes/test_attributes.py +++ b/opentelemetry-api/tests/attributes/test_attributes.py @@ -4,6 +4,8 @@ # type: ignore import copy +import logging +import threading import unittest import unittest.mock from collections.abc import MutableSequence @@ -269,19 +271,47 @@ def test_immutable(self): with self.assertRaises(TypeError): bdict["should-not-work"] = "dict immutable" - def test_locking(self): - """Supporting test case for a commit titled: Fix class BoundedAttributes to have RLock rather than Lock. See #3858. - The change was introduced because __iter__ of the class BoundedAttributes holds lock, and we observed some deadlock symptoms - in the codebase. This test case is to verify that the fix works as expected. + def test_no_deadlock_on_reentrant_logging(self): + """Regression test for #3858. + + The deadlock scenario: a logging handler intercepts the warning + emitted by _clean_attribute for an invalid value and calls __setitem__ + on the same BoundedAttributes instance from the same thread. + With _clean_attribute called inside the lock this caused a deadlock. + With _clean_attribute called before the lock is acquired, no deadlock + occurs. """ bdict = BoundedAttributes(immutable=False) - with bdict._lock: # pylint: disable=protected-access - for num in range(100): - bdict[str(num)] = num - - for num in range(100): - self.assertEqual(bdict[str(num)], num) + class ReentrantHandler(logging.Handler): + def emit(self, _record): + # Simulates Sentry intercepting the OTel warning and writing + # back into the same BoundedAttributes on the same thread. + bdict["reentrant.key"] = "set_by_handler" + + otel_logger = logging.getLogger("opentelemetry.attributes") + handler = ReentrantHandler() + otel_logger.addHandler(handler) + try: + completed = threading.Event() + + def run(): + # None is an invalid attribute value and triggers _logger.warning + # in _clean_attribute, which fires the ReentrantHandler above. + bdict["trigger.key"] = None + completed.set() + + thread = threading.Thread(target=run, daemon=True) + thread.start() + thread.join(timeout=2.0) + + self.assertTrue( + completed.is_set(), + "Deadlock detected: __setitem__ did not complete within 2s", + ) + self.assertEqual(bdict.get("reentrant.key"), "set_by_handler") + finally: + otel_logger.removeHandler(handler) # pylint: disable=no-self-use def test_extended_attributes(self):