From 7d964b12db90641d01cf56915152798762ac7d98 Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Wed, 17 Apr 2019 13:14:48 -0400 Subject: [PATCH] Context managers need to clean up properly An @contextmanager will raise an exception from its yield statement if an exception happens in the with-block that uses it. If the context manager needs to do clean up, it should do it even if an exception is raised, so it needs to be done in a finally clause. --- common/djangoapps/util/db.py | 8 +++++--- common/test/utils.py | 6 ++++-- lms/djangoapps/courseware/field_overrides.py | 6 ++++-- lms/djangoapps/grades/api/v1/gradebook_views.py | 8 +++++--- lms/djangoapps/grades/api/v1/views.py | 6 ++++-- lms/djangoapps/instructor_task/tests/test_tasks_helper.py | 6 ++++-- openedx/core/djangoapps/safe_sessions/middleware.py | 1 - .../management/tests/test_bulk_user_org_email_optout.py | 6 ++++-- 8 files changed, 30 insertions(+), 17 deletions(-) diff --git a/common/djangoapps/util/db.py b/common/djangoapps/util/db.py index c3386eabe2..7dea56aaef 100644 --- a/common/djangoapps/util/db.py +++ b/common/djangoapps/util/db.py @@ -164,9 +164,11 @@ def enable_named_outer_atomic(*names): for name in names: cache[name] = True - yield - for name in names: - del cache[name] + try: + yield + finally: + for name in names: + del cache[name] class OuterAtomic(transaction.Atomic): diff --git a/common/test/utils.py b/common/test/utils.py index 6262f4180a..860314a42b 100644 --- a/common/test/utils.py +++ b/common/test/utils.py @@ -108,8 +108,10 @@ def skip_signal(signal, **kwargs): and then reconnecting the signal. """ signal.disconnect(**kwargs) - yield - signal.connect(**kwargs) + try: + yield + finally: + signal.connect(**kwargs) class MockS3Mixin(object): diff --git a/lms/djangoapps/courseware/field_overrides.py b/lms/djangoapps/courseware/field_overrides.py index d2c3d93ffa..f2da813216 100644 --- a/lms/djangoapps/courseware/field_overrides.py +++ b/lms/djangoapps/courseware/field_overrides.py @@ -77,8 +77,10 @@ def disable_overrides(): """ prev = _OVERRIDES_DISABLED.disabled _OVERRIDES_DISABLED.disabled += (True,) - yield - _OVERRIDES_DISABLED.disabled = prev + try: + yield + finally: + _OVERRIDES_DISABLED.disabled = prev def overrides_disabled(): diff --git a/lms/djangoapps/grades/api/v1/gradebook_views.py b/lms/djangoapps/grades/api/v1/gradebook_views.py index a0cbb0402b..c9fb1b372f 100644 --- a/lms/djangoapps/grades/api/v1/gradebook_views.py +++ b/lms/djangoapps/grades/api/v1/gradebook_views.py @@ -71,9 +71,11 @@ def bulk_gradebook_view_context(course_key, users): CourseEnrollment.bulk_fetch_enrollment_states(users, course_key) cohorts.bulk_cache_cohorts(course_key, users) BulkRoleCache.prefetch(users) - yield - PersistentSubsectionGrade.clear_prefetched_data(course_key) - PersistentCourseGrade.clear_prefetched_data(course_key) + try: + yield + finally: + PersistentSubsectionGrade.clear_prefetched_data(course_key) + PersistentCourseGrade.clear_prefetched_data(course_key) def verify_writable_gradebook_enabled(view_func): diff --git a/lms/djangoapps/grades/api/v1/views.py b/lms/djangoapps/grades/api/v1/views.py index 7389cd7898..2d9c82375b 100644 --- a/lms/djangoapps/grades/api/v1/views.py +++ b/lms/djangoapps/grades/api/v1/views.py @@ -35,8 +35,10 @@ def bulk_course_grade_context(course_key, users): on context exit. """ PersistentCourseGrade.prefetch(course_key, users) - yield - PersistentCourseGrade.clear_prefetched_data(course_key) + try: + yield + finally: + PersistentCourseGrade.clear_prefetched_data(course_key) class CourseGradesView(GradeViewMixin, PaginatedAPIView): diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index fbf8047118..e7c3ae82ac 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -501,8 +501,10 @@ class TestProblemResponsesReport(TestReportMixin, InstructorTaskModuleTestCase): from xmodule.capa_module import CapaDescriptor generate_report_data = CapaDescriptor.generate_report_data del CapaDescriptor.generate_report_data - yield - CapaDescriptor.generate_report_data = generate_report_data + try: + yield + finally: + CapaDescriptor.generate_report_data = generate_report_data @patch.dict('django.conf.settings.FEATURES', {'MAX_PROBLEM_RESPONSES_COUNT': 4}) def test_build_student_data_limit(self): diff --git a/openedx/core/djangoapps/safe_sessions/middleware.py b/openedx/core/djangoapps/safe_sessions/middleware.py index 19a91698ab..effa8135bc 100644 --- a/openedx/core/djangoapps/safe_sessions/middleware.py +++ b/openedx/core/djangoapps/safe_sessions/middleware.py @@ -505,7 +505,6 @@ def controlled_logging(request, logger): try: yield - finally: if from_logout: logger.setLevel(default_level) diff --git a/openedx/core/djangoapps/user_api/management/tests/test_bulk_user_org_email_optout.py b/openedx/core/djangoapps/user_api/management/tests/test_bulk_user_org_email_optout.py index 7a33a0f5ef..b65d325cde 100644 --- a/openedx/core/djangoapps/user_api/management/tests/test_bulk_user_org_email_optout.py +++ b/openedx/core/djangoapps/user_api/management/tests/test_bulk_user_org_email_optout.py @@ -26,8 +26,10 @@ def _create_test_csv(csv_data): __, file_name = tempfile.mkstemp(text=True) with open(file_name, 'w') as file_pointer: file_pointer.write(csv_data.encode('utf-8')) - yield file_name - os.unlink(file_name) + try: + yield file_name + finally: + os.unlink(file_name) @mock.patch('openedx.core.djangoapps.user_api.management.commands.bulk_user_org_email_optout.log.info')