From 28f8248e833fb3122447e7822cab0e2003867d00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A1bor=20Boros?= Date: Mon, 8 Mar 2021 20:45:53 +0100 Subject: [PATCH 1/5] fix: address VisibleBlocks caching race condition --- lms/djangoapps/grades/models.py | 33 ++++++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/lms/djangoapps/grades/models.py b/lms/djangoapps/grades/models.py index ba67f8ac838c..79601bc8c246 100644 --- a/lms/djangoapps/grades/models.py +++ b/lms/djangoapps/grades/models.py @@ -16,7 +16,7 @@ from hashlib import sha1 from django.apps import apps -from django.db import models +from django.db import models, IntegrityError from django.utils.encoding import python_2_unicode_compatible from django.utils.timezone import now from lazy import lazy @@ -209,16 +209,39 @@ def bulk_create(cls, user_id, course_key, block_record_lists): for the block records' course with the new VisibleBlocks. Returns the newly created visible blocks. """ - created = cls.objects.bulk_create([ + visual_blocks = [ VisibleBlocks( blocks_json=brl.json_value, hashed=brl.hash_value, course_id=course_key, ) for brl in block_record_lists - ]) - cls._update_cache(user_id, course_key, created) - return created + ] + + created_visual_blocks = [] + existing_visual_blocks = [] + + try: + # Try to bulk create the blocks assuming all blocks are new + created_visual_blocks = cls.objects.bulk_create(visual_blocks) + except IntegrityError: + # Try to create blocks one by one and mark newly created blocks + for visual_block in visual_blocks: + existing_blocks = cls.objects.filter(hashed=visual_block.hashed) + + if existing_blocks.exists(): + # As only one record has a matching hash it is safe to use first + existing_visual_blocks.append(existing_blocks.first()) + else: + # Create the visual block and add mark as newly created + visual_block.save() + created_visual_blocks.append(visual_block) + + # Update the cache with the conjunction of created and existing blocks + cls._update_cache(user_id, course_key, existing_visual_blocks + created_visual_blocks) + + # Return the new visual blocks + return created_visual_blocks @classmethod def bulk_get_or_create(cls, user_id, course_key, block_record_lists): From eaa803e5899443033852b27c86e0d88e1c207207 Mon Sep 17 00:00:00 2001 From: Raul Gallegos Date: Fri, 16 Apr 2021 00:32:11 -0500 Subject: [PATCH 2/5] sets visual block creation in an atomic transaction --- lms/djangoapps/grades/models.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/grades/models.py b/lms/djangoapps/grades/models.py index 79601bc8c246..0e08650aea9c 100644 --- a/lms/djangoapps/grades/models.py +++ b/lms/djangoapps/grades/models.py @@ -16,7 +16,7 @@ from hashlib import sha1 from django.apps import apps -from django.db import models, IntegrityError +from django.db import models, IntegrityError, transaction from django.utils.encoding import python_2_unicode_compatible from django.utils.timezone import now from lazy import lazy @@ -223,7 +223,8 @@ def bulk_create(cls, user_id, course_key, block_record_lists): try: # Try to bulk create the blocks assuming all blocks are new - created_visual_blocks = cls.objects.bulk_create(visual_blocks) + with transaction.atomic(): + created_visual_blocks = cls.objects.bulk_create(visual_blocks) except IntegrityError: # Try to create blocks one by one and mark newly created blocks for visual_block in visual_blocks: From e45c4557a5f00e638c02408f781f106f95304f68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A1bor=20Boros?= Date: Tue, 4 May 2021 22:33:33 +0200 Subject: [PATCH 3/5] refactor: add logging statement to bulk create --- lms/djangoapps/grades/models.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lms/djangoapps/grades/models.py b/lms/djangoapps/grades/models.py index 0e08650aea9c..a296c128d48e 100644 --- a/lms/djangoapps/grades/models.py +++ b/lms/djangoapps/grades/models.py @@ -226,6 +226,11 @@ def bulk_create(cls, user_id, course_key, block_record_lists): with transaction.atomic(): created_visual_blocks = cls.objects.bulk_create(visual_blocks) except IntegrityError: + log.warning( + 'Falling back to create VisualBlocks one by one for user %s in course %s.' + % (user_id, course_key) + ) + # Try to create blocks one by one and mark newly created blocks for visual_block in visual_blocks: existing_blocks = cls.objects.filter(hashed=visual_block.hashed) From f2a528975cec877b165b095cf9a9e678a4ea6538 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A1bor=20Boros?= Date: Wed, 5 May 2021 00:19:09 +0200 Subject: [PATCH 4/5] tests: fix quality issue --- lms/djangoapps/grades/models.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lms/djangoapps/grades/models.py b/lms/djangoapps/grades/models.py index a296c128d48e..010d8861d380 100644 --- a/lms/djangoapps/grades/models.py +++ b/lms/djangoapps/grades/models.py @@ -226,10 +226,10 @@ def bulk_create(cls, user_id, course_key, block_record_lists): with transaction.atomic(): created_visual_blocks = cls.objects.bulk_create(visual_blocks) except IntegrityError: - log.warning( - 'Falling back to create VisualBlocks one by one for user %s in course %s.' - % (user_id, course_key) - ) + log.warning('Falling back to create VisualBlocks one by one for user %s in course %s.' % ( + user_id, + course_key + )) # Try to create blocks one by one and mark newly created blocks for visual_block in visual_blocks: From cf0cce48e80cadfbcbcdb781540113d6d051db70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A1bor=20Boros?= Date: Wed, 5 May 2021 09:42:46 +0200 Subject: [PATCH 5/5] style: use format in logging --- lms/djangoapps/grades/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/grades/models.py b/lms/djangoapps/grades/models.py index 010d8861d380..c45c007ca8b8 100644 --- a/lms/djangoapps/grades/models.py +++ b/lms/djangoapps/grades/models.py @@ -226,7 +226,7 @@ def bulk_create(cls, user_id, course_key, block_record_lists): with transaction.atomic(): created_visual_blocks = cls.objects.bulk_create(visual_blocks) except IntegrityError: - log.warning('Falling back to create VisualBlocks one by one for user %s in course %s.' % ( + log.warning('Falling back to create VisualBlocks one by one for user {} in course {}'.format( user_id, course_key ))