From d7825b26c4bb5cdff325fdc2273f757744b7b50d Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Sun, 11 Jan 2026 12:24:25 -0500 Subject: [PATCH] refactor: convert global modulestore to request-cached Having a global modulestore reference is a holdover from the early days, when the XML modulestore would be extremely expensive to initialize. That is no longer the case, and we should eliminate the global here to reduce the chances of multi-threading bugs that might mutate the global. Unfortunately, a lot of code currently assumes that calls to the modulestore() function are essentially free, instead of the 1-2 ms it is without caching. There are cases where this may be called thousands of times deep in loops somewhere while doing course content traversal, so it's risky to eliminate the caching behavior altogether. The thought here is that we'll tie to the RequestCache, which should at least not be shared across users/threads. Since new instances of the modulestore will not have cached entries for a particular course, it means that some query count tests have to be adjusted upward. --- .../transformers/tests/test_milestones.py | 3 +- .../grades/tests/test_course_grade_factory.py | 2 +- .../user_api/accounts/tests/test_utils.py | 4 +- xmodule/modulestore/django.py | 48 +++++++++---------- 4 files changed, 27 insertions(+), 30 deletions(-) diff --git a/lms/djangoapps/course_api/blocks/transformers/tests/test_milestones.py b/lms/djangoapps/course_api/blocks/transformers/tests/test_milestones.py index 5efea79c1986..6bd4fae414fd 100644 --- a/lms/djangoapps/course_api/blocks/transformers/tests/test_milestones.py +++ b/lms/djangoapps/course_api/blocks/transformers/tests/test_milestones.py @@ -44,6 +44,7 @@ def setUp(self): CourseEnrollmentFactory.create(user=self.user, course_id=self.course.id, is_active=True) self.transformers = BlockStructureTransformers([self.TRANSFORMER_CLASS_TO_TEST(False)]) + self.clear_caches() def setup_gated_section(self, gated_block, gating_block): """ @@ -184,7 +185,7 @@ def test_gated(self, gated_block_ref, gating_block_ref, expected_blocks_before_c self.user, ) - with self.assertNumQueries(4): + with self.assertNumQueries(5): self.get_blocks_and_check_against_expected(self.user, self.ALL_BLOCKS_EXCEPT_SPECIAL) def test_staff_access(self): diff --git a/lms/djangoapps/grades/tests/test_course_grade_factory.py b/lms/djangoapps/grades/tests/test_course_grade_factory.py index d7d3a20c1ff0..f05d29cadbca 100644 --- a/lms/djangoapps/grades/tests/test_course_grade_factory.py +++ b/lms/djangoapps/grades/tests/test_course_grade_factory.py @@ -289,7 +289,7 @@ def test_grading_exception(self, mock_course_grade): else mock_course_grade.return_value for student in self.students ] - with self.assertNumQueries(20): + with self.assertNumQueries(21): all_course_grades, all_errors = self._course_grades_and_errors_for(self.course, self.students) assert {student: str(all_errors[student]) for student in all_errors} == { student3: 'Error for student3.', diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_utils.py b/openedx/core/djangoapps/user_api/accounts/tests/test_utils.py index 9593dda0569a..107a5286f0e2 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_utils.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_utils.py @@ -11,7 +11,7 @@ from openedx.core.djangolib.testing.utils import skip_unless_lms from common.djangoapps.student.models import CourseEnrollment from common.djangoapps.student.tests.factories import UserFactory -from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.tests.factories import CourseFactory, BlockFactory # lint-amnesty, pylint: disable=wrong-import-order from ..utils import format_social_link, validate_social_link @@ -63,7 +63,7 @@ def test_social_link_input(self, platform_name, link_input, formatted_link_expec @ddt.ddt -class CompletionUtilsTestCase(SharedModuleStoreTestCase, CompletionWaffleTestMixin, TestCase): +class CompletionUtilsTestCase(ModuleStoreTestCase, CompletionWaffleTestMixin, TestCase): """ Test completion utility functions """ diff --git a/xmodule/modulestore/django.py b/xmodule/modulestore/django.py index c15f03a4e33d..1ded932f5bc8 100644 --- a/xmodule/modulestore/django.py +++ b/xmodule/modulestore/django.py @@ -24,8 +24,9 @@ import django.dispatch # lint-amnesty, pylint: disable=wrong-import-position import django.utils # lint-amnesty, pylint: disable=wrong-import-position from django.utils.translation import get_language, to_locale # lint-amnesty, pylint: disable=wrong-import-position -from edx_django_utils.cache import DEFAULT_REQUEST_CACHE # lint-amnesty, pylint: disable=wrong-import-position +from edx_django_utils.cache import RequestCache # lint-amnesty, pylint: disable=wrong-import-position +from openedx.core.lib.cache_utils import request_cached # pylint: disable=wrong-import-position from xmodule.contentstore.django import contentstore # lint-amnesty, pylint: disable=wrong-import-position from xmodule.modulestore.draft_and_published import BranchSettingMixin # lint-amnesty, pylint: disable=wrong-import-position from xmodule.modulestore.mixed import MixedModuleStore # lint-amnesty, pylint: disable=wrong-import-position @@ -55,6 +56,8 @@ log = logging.getLogger(__name__) ASSET_IGNORE_REGEX = getattr(settings, "ASSET_IGNORE_REGEX", r"(^\._.*$)|(^\.DS_Store$)|(^.*~$)") +MODULESTORE_REQUEST_CACHE_NAMESPACE = "modulestore" + class SwitchedSignal(django.dispatch.Signal): """ @@ -275,7 +278,7 @@ def create_modulestore_instance( if key in _options and isinstance(_options[key], str): _options[key] = load_function(_options[key]) - request_cache = DEFAULT_REQUEST_CACHE + request_cache = RequestCache(MODULESTORE_REQUEST_CACHE_NAMESPACE) try: metadata_inheritance_cache = caches['mongo_metadata_inheritance'] @@ -324,33 +327,27 @@ def fetch_disabled_xblock_types(): ) -# A singleton instance of the Mixed Modulestore -_MIXED_MODULESTORE = None - - +@request_cached(MODULESTORE_REQUEST_CACHE_NAMESPACE) def modulestore(): """ Returns the Mixed modulestore """ - global _MIXED_MODULESTORE # pylint: disable=global-statement - if _MIXED_MODULESTORE is None: - _MIXED_MODULESTORE = create_modulestore_instance( - settings.MODULESTORE['default']['ENGINE'], - contentstore(), - settings.MODULESTORE['default'].get('DOC_STORE_CONFIG', {}), - settings.MODULESTORE['default'].get('OPTIONS', {}) - ) - - if settings.FEATURES.get('CUSTOM_COURSES_EDX'): - # TODO: This import prevents a circular import issue, but is - # symptomatic of a lib having a dependency on code in lms. This - # should be updated to have a setting that enumerates modulestore - # wrappers and then uses that setting to wrap the modulestore in - # appropriate wrappers depending on enabled features. - from lms.djangoapps.ccx.modulestore import CCXModulestoreWrapper - _MIXED_MODULESTORE = CCXModulestoreWrapper(_MIXED_MODULESTORE) + mixed_modulestore = create_modulestore_instance( + settings.MODULESTORE['default']['ENGINE'], + contentstore(), + settings.MODULESTORE['default'].get('DOC_STORE_CONFIG', {}), + settings.MODULESTORE['default'].get('OPTIONS', {}) + ) + if settings.FEATURES.get('CUSTOM_COURSES_EDX'): + # TODO: This import prevents a circular import issue, but is + # symptomatic of a lib having a dependency on code in lms. This + # should be updated to have a setting that enumerates modulestore + # wrappers and then uses that setting to wrap the modulestore in + # appropriate wrappers depending on enabled features. + from lms.djangoapps.ccx.modulestore import CCXModulestoreWrapper + mixed_modulestore = CCXModulestoreWrapper(mixed_modulestore) - return _MIXED_MODULESTORE + return mixed_modulestore def clear_existing_modulestores(): @@ -360,8 +357,7 @@ def clear_existing_modulestores(): This is useful for flushing state between unit tests. """ - global _MIXED_MODULESTORE # pylint: disable=global-statement - _MIXED_MODULESTORE = None + RequestCache(MODULESTORE_REQUEST_CACHE_NAMESPACE).clear() class XBlockI18nService: