From db428e0adc4a53bb93daf0001c3536d53d1473b9 Mon Sep 17 00:00:00 2001 From: dzhuang Date: Tue, 5 Sep 2017 11:18:16 +0800 Subject: [PATCH] Make sure repo.close() --- course/content.py | 16 ++++---- course/flow.py | 89 ++++++++++++++++++++-------------------- course/models.py | 20 ++++----- course/utils.py | 51 +++++++++++++---------- course/versioning.py | 18 +++----- course/views.py | 17 ++++---- relate/utils.py | 24 +++++++++++ test/base_test_mixins.py | 34 +++++---------- 8 files changed, 141 insertions(+), 128 deletions(-) diff --git a/course/content.py b/course/content.py index 407d1ada..05106070 100644 --- a/course/content.py +++ b/course/content.py @@ -1479,14 +1479,14 @@ def get_course_commit_sha(course, participation): if participation.preview_git_commit_sha: preview_sha = participation.preview_git_commit_sha - repo = get_course_repo(course) - if isinstance(repo, SubdirRepoWrapper): - repo = repo.repo - - try: - repo[preview_sha.encode()] - except KeyError: - preview_sha = None + with get_course_repo(course) as repo: + if isinstance(repo, SubdirRepoWrapper): + repo = repo.repo + + try: + repo[preview_sha.encode()] + except KeyError: + preview_sha = None if preview_sha is not None: sha = preview_sha diff --git a/course/flow.py b/course/flow.py index 9122f446..c0324995 100644 --- a/course/flow.py +++ b/course/flow.py @@ -338,55 +338,54 @@ def grade_page_visit(visit, visit_grade_model=FlowPageVisitGrade, get_flow_page_desc, instantiate_flow_page) - repo = get_course_repo(course) + with get_course_repo(course) as repo: + course_commit_sha = get_course_commit_sha( + course, flow_session.participation if respect_preview else None) - course_commit_sha = get_course_commit_sha( - course, flow_session.participation if respect_preview else None) + flow_desc = get_flow_desc(repo, course, + flow_session.flow_id, course_commit_sha) - flow_desc = get_flow_desc(repo, course, - flow_session.flow_id, course_commit_sha) + page_desc = get_flow_page_desc( + flow_session.flow_id, + flow_desc, + page_data.group_id, page_data.page_id) - page_desc = get_flow_page_desc( - flow_session.flow_id, - flow_desc, - page_data.group_id, page_data.page_id) + page = instantiate_flow_page( + location="flow '%s', group, '%s', page '%s'" + % (flow_session.flow_id, page_data.group_id, page_data.page_id), + repo=repo, page_desc=page_desc, + commit_sha=course_commit_sha) - page = instantiate_flow_page( - location="flow '%s', group, '%s', page '%s'" - % (flow_session.flow_id, page_data.group_id, page_data.page_id), - repo=repo, page_desc=page_desc, - commit_sha=course_commit_sha) - - assert page.expects_answer() - if not page.is_answer_gradable(): - return - - from course.page import PageContext - grading_page_context = PageContext( - course=course, - repo=repo, - commit_sha=course_commit_sha, - flow_session=flow_session) - - with translation.override(settings.RELATE_ADMIN_EMAIL_LOCALE): - answer_feedback = page.grade( - grading_page_context, visit.page_data.data, - visit.answer, grade_data=grade_data) - - grade = visit_grade_model() - grade.visit = visit - grade.grade_data = grade_data - grade.max_points = page.max_points(visit.page_data) - grade.graded_at_git_commit_sha = course_commit_sha.decode() - - bulk_feedback_json = None - if answer_feedback is not None: - grade.correctness = answer_feedback.correctness - grade.feedback, bulk_feedback_json = answer_feedback.as_json() - - grade.save() - - update_bulk_feedback(page_data, grade, bulk_feedback_json) + assert page.expects_answer() + if not page.is_answer_gradable(): + return + + from course.page import PageContext + grading_page_context = PageContext( + course=course, + repo=repo, + commit_sha=course_commit_sha, + flow_session=flow_session) + + with translation.override(settings.RELATE_ADMIN_EMAIL_LOCALE): + answer_feedback = page.grade( + grading_page_context, visit.page_data.data, + visit.answer, grade_data=grade_data) + + grade = visit_grade_model() + grade.visit = visit + grade.grade_data = grade_data + grade.max_points = page.max_points(visit.page_data) + grade.graded_at_git_commit_sha = course_commit_sha.decode() + + bulk_feedback_json = None + if answer_feedback is not None: + grade.correctness = answer_feedback.correctness + grade.feedback, bulk_feedback_json = answer_feedback.as_json() + + grade.save() + + update_bulk_feedback(page_data, grade, bulk_feedback_json) # }}} diff --git a/course/models.py b/course/models.py index 6a0f84c3..3bc1d4ff 100644 --- a/course/models.py +++ b/course/models.py @@ -1280,16 +1280,16 @@ class FlowRuleException(models.Model): from relate.utils import dict_to_struct rule = dict_to_struct(self.rule) - repo = get_course_repo(self.participation.course) - commit_sha = get_course_commit_sha( - self.participation.course, self.participation) - ctx = ValidationContext( - repo=repo, - commit_sha=commit_sha) - - flow_desc = get_flow_desc(repo, - self.participation.course, - self.flow_id, commit_sha) + with get_course_repo(self.participation.course) as repo: + commit_sha = get_course_commit_sha( + self.participation.course, self.participation) + ctx = ValidationContext( + repo=repo, + commit_sha=commit_sha) + + flow_desc = get_flow_desc(repo, + self.participation.course, + self.flow_id, commit_sha) tags = None grade_identifier = None diff --git a/course/utils.py b/course/utils.py index 2ff1f3f5..f6c172a3 100644 --- a/course/utils.py +++ b/course/utils.py @@ -577,24 +577,25 @@ class CoursePageContext(object): if self.participation.preview_git_commit_sha: preview_sha = self.participation.preview_git_commit_sha.encode() - repo = get_course_repo(self.course) - - from relate.utils import SubdirRepoWrapper - if isinstance(repo, SubdirRepoWrapper): - true_repo = repo.repo - else: - true_repo = cast(dulwich.repo.Repo, repo) - - try: - true_repo[preview_sha] - except KeyError: - from django.contrib import messages - messages.add_message(request, messages.ERROR, - _("Preview revision '%s' does not exist--" - "showing active course content instead.") - % preview_sha.decode()) - - preview_sha = None + with get_course_repo(self.course) as repo: + from relate.utils import SubdirRepoWrapper + if isinstance(repo, SubdirRepoWrapper): + true_repo = repo.repo + else: + true_repo = cast(dulwich.repo.Repo, repo) + + try: + true_repo[preview_sha] + except KeyError: + from django.contrib import messages + messages.add_message(request, messages.ERROR, + _("Preview revision '%s' does not exist--" + "showing active course content instead.") + % preview_sha.decode()) + + preview_sha = None + finally: + true_repo.close() if preview_sha is not None: sha = preview_sha @@ -634,6 +635,12 @@ class CoursePageContext(object): else: return (perm, argument) in self.permissions() + def __enter__(self): + return self + + def __exit__(self, exc_type, exc_val, exc_tb): + self.repo.close() + class FlowContext(object): def __init__(self, repo, course, flow_id, participation=None): @@ -752,10 +759,10 @@ def instantiate_flow_page_with_ctx(fctx, page_data): # {{{ utilties for course-based views def course_view(f): def wrapper(request, course_identifier, *args, **kwargs): - pctx = CoursePageContext(request, course_identifier) - response = f(pctx, *args, **kwargs) - pctx.repo.close() - return response + with CoursePageContext(request, course_identifier) as pctx: + response = f(pctx, *args, **kwargs) + pctx.repo.close() + return response from functools import update_wrapper update_wrapper(wrapper, f) diff --git a/course/versioning.py b/course/versioning.py index 68eac1ba..2c9c6ca7 100644 --- a/course/versioning.py +++ b/course/versioning.py @@ -281,24 +281,14 @@ def set_up_new_course(request): # Don't coalesce this handler with the one below. We only want # to delete the directory if we created it. Trust me. - # Work around read-only files on Windows. - # https://docs.python.org/3.5/library/shutil.html#rmtree-example - - import os - import stat - import shutil - # Make sure files opened for 'repo' above are actually closed. if repo is not None: # noqa repo.close() # noqa - def remove_readonly(func, path, _): # noqa - "Clear the readonly bit and reattempt the removal" - os.chmod(path, stat.S_IWRITE) - func(path) + from relate.utils import force_remove_path try: - shutil.rmtree(repo_path, onerror=remove_readonly) + force_remove_path(repo_path) except OSError: messages.add_message(request, messages.WARNING, ugettext("Failed to delete unused " @@ -307,6 +297,10 @@ def set_up_new_course(request): raise + finally: + if repo is not None: + repo.close() + except Exception as e: from traceback import print_exc print_exc() diff --git a/course/views.py b/course/views.py index 5128d3e2..bf9c9334 100644 --- a/course/views.py +++ b/course/views.py @@ -252,8 +252,9 @@ def media_etag_func(request, course_identifier, commit_sha, media_path): def get_media(request, course_identifier, commit_sha, media_path): course = get_object_or_404(Course, identifier=course_identifier) - repo = get_course_repo(course) - return get_repo_file_response(repo, "media/" + media_path, commit_sha.encode()) + with get_course_repo(course) as repo: + return get_repo_file_response( + repo, "media/" + media_path, commit_sha.encode()) def repo_file_etag_func(request, course_identifier, commit_sha, path): @@ -320,9 +321,6 @@ def get_repo_file_backend( # check to see if the course is hidden check_course_state(course, participation) - # retrieve local path for the repo for the course - repo = get_course_repo(course) - # set access to public (or unenrolled), student, etc if request.relate_exam_lockdown: access_kinds = ["in_exam"] @@ -335,10 +333,13 @@ def get_repo_file_backend( and arg is not None] from course.content import is_repo_file_accessible_as - if not is_repo_file_accessible_as(access_kinds, repo, commit_sha, path): - raise PermissionDenied() - return get_repo_file_response(repo, path, commit_sha) + # retrieve local path for the repo for the course + with get_course_repo(course) as repo: + if not is_repo_file_accessible_as(access_kinds, repo, commit_sha, path): + raise PermissionDenied() + + return get_repo_file_response(repo, path, commit_sha) def get_repo_file_response(repo, path, commit_sha): diff --git a/relate/utils.py b/relate/utils.py index 7dbae3f4..3a06b272 100644 --- a/relate/utils.py +++ b/relate/utils.py @@ -115,6 +115,12 @@ class SubdirRepoWrapper(object): def close(self): self.repo.close() + def __enter__(self): + return self + + def __exit__(self, exc_type, exc_val, exc_tb): + self.close() + Repo_ish = Union[dulwich.repo.Repo, SubdirRepoWrapper] @@ -425,4 +431,22 @@ def ignore_no_such_table(f, *args): raise +def force_remove_path(path): + # type: (Text) -> None + """ + Work around deleting read-only path on Windows. + Ref: https://docs.python.org/3.5/library/shutil.html#rmtree-example + """ + import os + import stat + import shutil + + def remove_readonly(func, path, _): # noqa + "Clear the readonly bit and reattempt the removal" + os.chmod(path, stat.S_IWRITE) + func(path) + + shutil.rmtree(path, onerror=remove_readonly) + + # vim: foldmethod=marker diff --git a/test/base_test_mixins.py b/test/base_test_mixins.py index 5d7a01cc..a9932c7e 100644 --- a/test/base_test_mixins.py +++ b/test/base_test_mixins.py @@ -27,6 +27,7 @@ from django.conf import settings from django.test import Client from django.urls import reverse from django.contrib.auth import get_user_model +from relate.utils import force_remove_path from course.models import Course, Participation, ParticipationRole from course.constants import participation_status @@ -90,25 +91,6 @@ SINGLE_COURSE_SETUP_LIST = [ ] -def force_remove_path(path): - # shutil.rmtree won't work when delete course repo folder, on Windows, - # so it cause all testcases failed. - # Though this work around (copied from http://bit.ly/2usqGxr) still fails - # for some tests, this enables **some other** tests on Windows. - import stat - def remove_readonly(func, path, _): # noqa - os.chmod(path, stat.S_IWRITE) - func(path) - - import shutil - try: - shutil.rmtree(path, onerror=remove_readonly) - except OSError: - # let the remove_exceptionally_undelete_course_repos method to delete - # the folder for the next test. - pass - - class SuperuserCreateMixin(object): create_superuser_kwargs = CREATE_SUPERUSER_KWARGS @@ -178,12 +160,18 @@ class CoursesTestMixinBase(SuperuserCreateMixin): @classmethod def remove_exceptionally_undelete_course_repos(cls, course_identifier): - # Remove undelete course repo folders coursed by - # unexpected exceptions in previous tests. + """ + Remove existing course repo folders resulted in unexpected + exceptions in previous tests. + """ + repo_path = os.path.join(settings.GIT_ROOT, course_identifier) try: - force_remove_path(os.path.join(settings.GIT_ROOT, course_identifier)) + force_remove_path(repo_path) except OSError: - pass + if not os.path.isdir(repo_path): + # The repo path does not exist, that's good! + return + raise @classmethod def remove_course_repo(cls, course): -- GitLab