Merge pull request #12481 from edx/fix-memory-leak

clear request cache after celery task finishes (TNL-2705)

cms/djangoapps/contentstore/views/tests/test_group_configurations.py

@@ -88,7 +88,7 @@ class HelperMethods(object):
         self.save_course()
         return (vertical, split_test)
 
-    def _create_problem_with_content_group(self, cid, group_id, name_suffix='', special_characters=''):
+    def _create_problem_with_content_group(self, cid, group_id, name_suffix='', special_characters='', orphan=False):
         """
         Create a problem
         Assign content group to the problem.
@@ -112,6 +112,7 @@ class HelperMethods(object):
             data={'metadata': group_access_content}
         )
 
+        if not orphan:
             self.course.children.append(vertical.location)
         self.save_course()
 
@@ -682,24 +683,16 @@ class GroupConfigurationsUsageInfoTestCase(CourseTestCase, HelperMethods):
     @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
     def test_can_get_correct_usage_info_with_orphan(self, module_store_type):
         """
-        Test if content group json updated successfully with usage information even if there is
-        an orphan in content group.
+        Test if content group json updated successfully with usage information
+        even if there is an orphan in content group.
         """
         self.course = CourseFactory.create(default_store=module_store_type)
         self._add_user_partitions(count=1, scheme_id='cohort')
-        vertical, problem = self._create_problem_with_content_group(cid=0, group_id=1, name_suffix='0')
+        vertical, __ = self._create_problem_with_content_group(cid=0, group_id=1, name_suffix='0', orphan=True)
 
-        # Assert that there is no orphan in the course yet.
-        self.assertEqual(len(self.store.get_orphans(self.course.id)), 0)
-
-        # Update problem(created earlier) to an orphan.
-        with self.store.branch_setting(ModuleStoreEnum.Branch.published_only):
-            vertical = self.store.get_item(vertical.location)
-            vertical.children.remove(problem.location)
-            self.store.update_item(vertical, self.user.id)
-
-        # Assert that the problem is orphan now.
-        self.assertIn(problem.location, self.store.get_orphans(self.course.id))
+        # Assert that there is an orphan in the course, and that it's the vertical
+        self.assertEqual(len(self.store.get_orphans(self.course.id)), 1)
+        self.assertIn(vertical.location, self.store.get_orphans(self.course.id))
 
         # Get the expected content group information based on module store.
         if module_store_type == ModuleStoreEnum.Type.mongo:

common/djangoapps/request_cache/__init__.py

@@ -8,6 +8,7 @@ is installed in order to clear the cache after each request.
 import logging
 from urlparse import urlparse
 
+from celery.signals import task_postrun
 from django.conf import settings
 from django.test.client import RequestFactory
 
@@ -17,6 +18,15 @@ from request_cache import middleware
 log = logging.getLogger(__name__)
 
 
+@task_postrun.connect
+def clear_request_cache(**kwargs):  # pylint: disable=unused-argument
+    """
+    Once a celery task completes, clear the request cache to
+    prevent memory leaks.
+    """
+    middleware.RequestCache.clear_request_cache()
+
+
 def get_cache(name):
     """
     Return the request cache named ``name``.

common/djangoapps/request_cache/tests.py

 """
 Tests for the request cache.
 """
+from celery.task import task
 from django.conf import settings
 from django.test import TestCase
 
 from request_cache import get_request_or_stub
+from xmodule.modulestore.django import modulestore
 
 
 class TestRequestCache(TestCase):
@@ -13,8 +15,21 @@ class TestRequestCache(TestCase):
     """
 
     def test_get_request_or_stub(self):
-        # Outside the context of the request, we should still get a request
-        # that allows us to build an absolute URI.
+        """
+        Outside the context of the request, we should still get a request
+        that allows us to build an absolute URI.
+        """
         stub = get_request_or_stub()
         expected_url = "http://{site_name}/foobar".format(site_name=settings.SITE_NAME)
         self.assertEqual(stub.build_absolute_uri("foobar"), expected_url)
+
+    @task
+    def _dummy_task(self):
+        """ Create a task that adds stuff to the request cache. """
+        cache = {"course_cache": "blah blah blah"}
+        modulestore().request_cache.data.update(cache)
+
+    def test_clear_cache_celery(self):
+        """ Test that the request cache is cleared after a task is run. """
+        self._dummy_task.apply(args=(self,)).get()
+        self.assertEqual(modulestore().request_cache.data, {})

common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py

@@ -630,8 +630,8 @@ class SplitModuleCourseTests(SplitModuleTest):
     @patch('xmodule.tabs.CourseTab.from_json', side_effect=mock_tab_from_json)
     def test_get_courses_with_same_course_index(self, _from_json):
         """
-        Test that if two courses pointing to same course index,
-        get_courses should return both.
+        Test that if two courses point to same course index,
+        `get_courses` should return both courses.
         """
         courses = modulestore().get_courses(branch=BRANCH_NAME_DRAFT)
         # Should have gotten 3 draft courses.

openedx/core/djangoapps/bookmarks/tests/test_models.py

@@ -255,10 +255,10 @@ class BookmarkModelTests(BookmarksTestsBase):
 
     @ddt.data(
         (ModuleStoreEnum.Type.mongo, 'course', [], 3),
-        (ModuleStoreEnum.Type.mongo, 'chapter_1', [], 3),
-        (ModuleStoreEnum.Type.mongo, 'sequential_1', ['chapter_1'], 4),
-        (ModuleStoreEnum.Type.mongo, 'vertical_1', ['chapter_1', 'sequential_1'], 5),
-        (ModuleStoreEnum.Type.mongo, 'html_1', ['chapter_1', 'sequential_2', 'vertical_2'], 6),
+        (ModuleStoreEnum.Type.mongo, 'chapter_1', [], 4),
+        (ModuleStoreEnum.Type.mongo, 'sequential_1', ['chapter_1'], 6),
+        (ModuleStoreEnum.Type.mongo, 'vertical_1', ['chapter_1', 'sequential_1'], 8),
+        (ModuleStoreEnum.Type.mongo, 'html_1', ['chapter_1', 'sequential_2', 'vertical_2'], 10),
         (ModuleStoreEnum.Type.split, 'course', [], 3),
         (ModuleStoreEnum.Type.split, 'chapter_1', [], 2),
         (ModuleStoreEnum.Type.split, 'sequential_1', ['chapter_1'], 2),
@@ -268,8 +268,9 @@ class BookmarkModelTests(BookmarksTestsBase):
     @ddt.unpack
     def test_path_and_queries_on_create(self, store_type, block_to_bookmark, ancestors_attrs, expected_mongo_calls):
         """
-        In case of mongo, 1 query is used to fetch the block, and 2 by path_to_location(), and then
-        1 query per parent in path is needed to fetch the parent blocks.
+        In case of mongo, 1 query is used to fetch the block, and 2
+        by path_to_location(), and then 1 query per parent in path
+        is needed to fetch the parent blocks.
         """
 
         self.setup_test_data(store_type)