diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 85ee6cd6a..599ca96a2 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,13 @@ Change history for XBlock Unreleased ---------- +* Removed ``threading.RLock`` from ``Mixologist``'s class cache to make it safe + for ASGI/async deployments. ``dict.setdefault()`` atomicity (via CPython's GIL) + provides equivalent thread safety without blocking the event loop. + Relates to `openedx/openedx-platform#38680 `_. +* Deprecated ``ObjectAggregator`` from ``xblock.runtime``; it had no production + callers and will be removed in a future major release. + 6.2.0 - 2026-06-09 ------------------ diff --git a/xblock/runtime.py b/xblock/runtime.py index 8aa822dda..f7c4dcd84 100644 --- a/xblock/runtime.py +++ b/xblock/runtime.py @@ -11,7 +11,6 @@ import json import logging import re -import threading import warnings from lxml import etree @@ -1195,6 +1194,11 @@ class ObjectAggregator: """ def __init__(self, *objects): + warnings.warn( + "ObjectAggregator is deprecated and will be removed in a future release.", + DeprecationWarning, + stacklevel=2, + ) self.__dict__['_objects'] = objects def _object_with_attr(self, name): @@ -1221,9 +1225,11 @@ def __delattr__(self, name): delattr(self._object_with_attr(name), name) -# Cache of Mixologist generated classes +# Cache of Mixologist generated classes. +# dict.setdefault() is atomic under CPython's GIL, so no lock is needed here. +# This is also safe in asyncio contexts: mix() has no await points, so the +# event loop cannot interleave two calls to it within the same thread. _CLASS_CACHE = {} -_CLASS_CACHE_LOCK = threading.RLock() class Mixologist: @@ -1277,18 +1283,16 @@ def mix(self, cls): mixin_key = (base_class, mixins) if mixin_key not in _CLASS_CACHE: - # Only lock if we're about to make a new class - with _CLASS_CACHE_LOCK: - # Use setdefault so that if someone else has already - # created a class before we got the lock, we don't - # overwrite it - return _CLASS_CACHE.setdefault(mixin_key, type( - base_class.__name__ + 'WithMixins', # type() requires native str - (base_class,) + mixins, - {'unmixed_class': base_class} - )) - else: - return _CLASS_CACHE[mixin_key] + new_class = type( + base_class.__name__ + 'WithMixins', # type() requires native str + (base_class,) + mixins, + {'unmixed_class': base_class} + ) + # setdefault is atomic under CPython's GIL: if two threads race + # here, one wins and the other's new_class is silently discarded. + _CLASS_CACHE.setdefault(mixin_key, new_class) + + return _CLASS_CACHE[mixin_key] class RegexLexer: diff --git a/xblock/test/test_runtime.py b/xblock/test/test_runtime.py index 4e58d813f..d5350ba99 100644 --- a/xblock/test/test_runtime.py +++ b/xblock/test/test_runtime.py @@ -375,7 +375,10 @@ def __init__(self, **kwargs): class TestObjectAggregator: """ - Test that the ObjectAggregator behaves correctly + Test that the ObjectAggregator behaves correctly. + + ObjectAggregator is deprecated; all instantiations are wrapped to assert + the expected DeprecationWarning is raised. """ # pylint: disable=attribute-defined-outside-init def setup_method(self): @@ -385,7 +388,8 @@ def setup_method(self): # Create some objects that only have single attributes self.first = Dynamic(first=1) self.second = Dynamic(second=2) - self.agg = ObjectAggregator(self.first, self.second) + with pytest.warns(DeprecationWarning, match="ObjectAggregator is deprecated"): + self.agg = ObjectAggregator(self.first, self.second) # pylint: enable=attribute-defined-outside-init def test_get(self):