From f03b141f052758a10f44a542680801a44c5528fc Mon Sep 17 00:00:00 2001 From: Jeff Cohen Date: Mon, 7 Nov 2022 11:48:39 -0500 Subject: [PATCH] fix: do not clear request cache when running eager Celery tasks (#31261) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clearing the RequestCache was intended to address memory leaks in the celery workers. Celery worker processes will process many tasks before they are terminated. RequestCache cleanup typically happens in the RequestCacheMiddleware class, and middleware never executes for celery. To get around that issue, the CLEAR_REQUEST_CACHE_ON_TASK_COMPLETION setting was created to clear the RequestCache after every task was successfully completed. This works fine when celery is running as a separate process, as it's set up to do in production. But during development, the CELERY_ALWAYS_EAGER setting variable is set to True, meaning that celery tasks are run in the same thread as the Django Request. This is meant to make debugging easier, as task failures run as part of the request cycle and will raise exceptions that are visible to the browser. However, celery tasks are triggered from many different actions. That means that the RequestCache was being cleared many times during the course of processing a request. This led to behavior that was potentially slower, but also incorrect–the RequestCache was getting flushed in a way that wouldn't happen in any deployed environment because celery would be running in separate processes there. This came up when trying to fix an issue around extra history records being created during problem submissions: https://discuss.openedx.org/t/extra-history-record-stored-on-each-problem-submission/8081 Furthermore, it's not necessary to prevent RequestCache memory leaks when running in CELERY_AWLAYS_EAGER mode in development because the middleware cleanup happens automatically–as everything is running as part of the request/response cycle. There are times in which we may want to run celery eagerly and still clear the cache, such as testing. I have set CLEAR_REQUEST_CACHE_ON_TASK_COMPLETION = False in all dev and test environments that already have CELERY_ALWAYS_EAGER = True. The unit test that specifically tests whether the request cache is getting cleared upon completion of a celery task then overrides CLEAR_REQUEST_CACHE_ON_TASK_COMPLETION = True even though CELERY_ALWAYS_EAGER = True for the sake of that specific testing purpose. --- cms/envs/bok_choy.env.json | 1 + cms/envs/bok_choy.yml | 1 + cms/envs/bok_choy_docker.env.json | 1 + cms/envs/bok_choy_docker.yml | 1 + cms/envs/devstack.py | 5 +++++ cms/envs/production.py | 1 + lms/djangoapps/courseware/models.py | 14 ++++++++------ lms/envs/bok_choy.yml | 1 + lms/envs/devstack.py | 4 ++++ 9 files changed, 23 insertions(+), 6 deletions(-) diff --git a/cms/envs/bok_choy.env.json b/cms/envs/bok_choy.env.json index b40518b7d3..6f14115ab7 100644 --- a/cms/envs/bok_choy.env.json +++ b/cms/envs/bok_choy.env.json @@ -47,6 +47,7 @@ "CELERY_BROKER_HOSTNAME": "localhost", "CELERY_BROKER_TRANSPORT": "amqp", "CERT_QUEUE": "certificates", + "CLEAR_REQUEST_CACHE_ON_TASK_COMPLETION": false, "CMS_BASE": "localhost:8031", "CODE_JAIL": { "limits": { diff --git a/cms/envs/bok_choy.yml b/cms/envs/bok_choy.yml index 4c2f8b81de..b378cf6443 100644 --- a/cms/envs/bok_choy.yml +++ b/cms/envs/bok_choy.yml @@ -35,6 +35,7 @@ CELERY_BROKER_HOSTNAME: localhost CELERY_BROKER_PASSWORD: celery CELERY_BROKER_TRANSPORT: amqp CELERY_BROKER_USER: celery +CLEAR_REQUEST_CACHE_ON_TASK_COMPLETION: false CERT_QUEUE: certificates CMS_BASE: localhost:8031 CODE_JAIL: diff --git a/cms/envs/bok_choy_docker.env.json b/cms/envs/bok_choy_docker.env.json index 628e374856..6ab1fb3d66 100644 --- a/cms/envs/bok_choy_docker.env.json +++ b/cms/envs/bok_choy_docker.env.json @@ -44,6 +44,7 @@ } }, "CELERY_ALWAYS_EAGER": true, + "CLEAR_REQUEST_CACHE_ON_TASK_COMPLETION": false, "CELERY_BROKER_HOSTNAME": "localhost", "CELERY_BROKER_TRANSPORT": "amqp", "CERT_QUEUE": "certificates", diff --git a/cms/envs/bok_choy_docker.yml b/cms/envs/bok_choy_docker.yml index 03cc94f28f..829b116766 100644 --- a/cms/envs/bok_choy_docker.yml +++ b/cms/envs/bok_choy_docker.yml @@ -36,6 +36,7 @@ CELERY_BROKER_PASSWORD: celery CELERY_BROKER_TRANSPORT: amqp CELERY_BROKER_USER: celery CERT_QUEUE: certificates +CLEAR_REQUEST_CACHE_ON_TASK_COMPLETION: false CMS_BASE: '** OVERRIDDEN **' CODE_JAIL: limits: {REALTIME: 3, VMEM: 0} diff --git a/cms/envs/devstack.py b/cms/envs/devstack.py index c76c24bac8..bf4517f693 100644 --- a/cms/envs/devstack.py +++ b/cms/envs/devstack.py @@ -79,6 +79,11 @@ DJFS = { # By default don't use a worker, execute tasks as if they were local functions CELERY_ALWAYS_EAGER = True +# When the celery task is eagerly, it is executed locally while sharing the +# thread and its request cache with the active Django Request. In that case, +# do not clear the cache. +CLEAR_REQUEST_CACHE_ON_TASK_COMPLETION = False + ################################ DEBUG TOOLBAR ################################ INSTALLED_APPS += ['debug_toolbar'] diff --git a/cms/envs/production.py b/cms/envs/production.py index 6431ad95c1..c3c88abeb6 100644 --- a/cms/envs/production.py +++ b/cms/envs/production.py @@ -409,6 +409,7 @@ CELERY_BROKER_HOSTNAME = ENV_TOKENS.get("CELERY_BROKER_HOSTNAME", "") CELERY_BROKER_VHOST = ENV_TOKENS.get("CELERY_BROKER_VHOST", "") CELERY_BROKER_USER = AUTH_TOKENS.get("CELERY_BROKER_USER", "") CELERY_BROKER_PASSWORD = AUTH_TOKENS.get("CELERY_BROKER_PASSWORD", "") +CLEAR_REQUEST_CACHE_ON_TASK_COMPLETION = ENV_TOKENS.get("CLEAR_REQUEST_CACHE_ON_TASK_COMPLETION", True) BROKER_URL = "{}://{}:{}@{}/{}".format(CELERY_BROKER_TRANSPORT, CELERY_BROKER_USER, diff --git a/lms/djangoapps/courseware/models.py b/lms/djangoapps/courseware/models.py index 05020a811d..aebfcb1697 100644 --- a/lms/djangoapps/courseware/models.py +++ b/lms/djangoapps/courseware/models.py @@ -255,12 +255,14 @@ class StudentModuleHistory(BaseStudentModuleHistory): we save. """ if instance.module_type in StudentModuleHistory.HISTORY_SAVING_TYPES: - history_entry = StudentModuleHistory(student_module=instance, - version=None, - created=instance.modified, - state=instance.state, - grade=instance.grade, - max_grade=instance.max_grade) + history_entry = StudentModuleHistory( + student_module=instance, + version=None, + created=instance.modified, + state=instance.state, + grade=instance.grade, + max_grade=instance.max_grade + ) history_entry.save() # When the extended studentmodulehistory table exists, don't save diff --git a/lms/envs/bok_choy.yml b/lms/envs/bok_choy.yml index c49c34b437..7a0eb44f40 100644 --- a/lms/envs/bok_choy.yml +++ b/lms/envs/bok_choy.yml @@ -53,6 +53,7 @@ CELERY_BROKER_TRANSPORT: amqp CELERY_BROKER_USER: celery CELERY_ALWAYS_EAGER: True CELERY_RESULT_BACKEND: 'django-cache' +CLEAR_REQUEST_CACHE_ON_TASK_COMPLETION: False CERT_QUEUE: certificates CMS_BASE: localhost:8031 diff --git a/lms/envs/devstack.py b/lms/envs/devstack.py index d7c66cb202..bb7bb554b5 100644 --- a/lms/envs/devstack.py +++ b/lms/envs/devstack.py @@ -33,6 +33,10 @@ SESSION_COOKIE_NAME = 'lms_sessionid' # By default don't use a worker, execute tasks as if they were local functions CELERY_ALWAYS_EAGER = True +# When the celery task is run eagerly, it is executed locally while sharing the +# thread and its request cache with the active Django Request. In that case, +# do not clear the cache. +CLEAR_REQUEST_CACHE_ON_TASK_COMPLETION = False HTTPS = 'off' LMS_ROOT_URL = f'http://{LMS_BASE}'