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.

Author: ormsbee

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):

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.',

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
     """

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: