Skip to content

fix: make Mixologist async-safe, deprecate ObjectAggregator #934

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

Open
irfanuddinahmad wants to merge 1 commit into
base: master
Choose a base branch
from
+32 −17
Open

fix: make Mixologist async-safe, deprecate ObjectAggregator #934

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 7 additions & 0 deletions CHANGELOG.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 <https://github.com/openedx/openedx-platform/issues/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
------------------

Expand Down
34 changes: 19 additions & 15 deletions xblock/runtime.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
import json
import logging
import re
import threading
import warnings

from lxml import etree
Expand Down Expand Up @@ -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):
Expand All @@ -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:
Expand Down Expand Up @@ -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(

@kdmccormick kdmccormick Jul 1, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you both for reviewing.

@ormsbee , blocks are actually mixed on-demand, not at startup. If you throw a breakpoint here and start up a fresh CMS, it won't be hit until you load up a course, at which point you'll see it hit as part of the request handler once per loaded block type. Does that change your review at all?

@ormsbee ormsbee Jul 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't change my review, but it just seems like something that should happen at one time and place during startup (triggered from an AppConfig somewhere), unless there's a really compelling reason to delay it.

@ormsbee ormsbee Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(With the justification that the Mixologist engages in weird magic, and weird magic should ideally happen only once and in obvious place and time.) To be clear, I'm not asking for this PR to change this behavior, especially since that's likely to be risky.

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:
Expand Down
8 changes: 6 additions & 2 deletions xblock/test/test_runtime.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand All @@ -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):
Expand Down