Patchwork [1,of,7,STABLE] subrepo: ensure "lock.release()" execution at the end of "storeclean()"

login
register
mail settings
Submitter Katsunori FUJIWARA
Date June 19, 2014, 3:43 p.m.
Message ID <15bc43d3360a4826cab2.1403192596@juju>
Download mbox | patch
Permalink /patch/5017/
State Accepted
Commit fe9db58b0b2d1f0c36ba197c880150ad55ab9a19
Headers show

Comments

Katsunori FUJIWARA - June 19, 2014, 3:43 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1403191279 -32400
#      Fri Jun 20 00:21:19 2014 +0900
# Branch stable
# Node ID 15bc43d3360a4826cab2c3b93e6bf2cfafbcf070
# Parent  b2dc026a9bd2620a0b8c1f28caf5a608352960ab
subrepo: ensure "lock.release()" execution at the end of "storeclean()"

Before this patch, "lock.release()" for "self._repo" in "storeclean()"
of "hgsubrepo" may not be executed, if unexpected exception is raised,
because it isn't executed in "finally" clause.

This patch ensures "lock.release()" execution at the end of
"storeclean()" by moving it into "finally" clause.

This patch chooses moving almost all lines in "storeclean()" into
"_storeclean()" instead of indenting them for "try/finally" clauses,
to keep diff simple for review-ability.
Katsunori FUJIWARA - June 19, 2014, 3:52 p.m.
At Fri, 20 Jun 2014 00:43:16 +0900,
FUJIWARA Katsunori wrote:
> 
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1403191279 -32400
> #      Fri Jun 20 00:21:19 2014 +0900
> # Branch stable
> # Node ID 15bc43d3360a4826cab2c3b93e6bf2cfafbcf070
> # Parent  b2dc026a9bd2620a0b8c1f28caf5a608352960ab
> subrepo: ensure "lock.release()" execution at the end of "storeclean()"

This patch series is posted as STABLE for preventive bug fixing,
because it may be difficult to avoid them if they once occur.

But I also agree queuing them into DEFAULT, because of rare
possibility of these problems.


> Before this patch, "lock.release()" for "self._repo" in "storeclean()"
> of "hgsubrepo" may not be executed, if unexpected exception is raised,
> because it isn't executed in "finally" clause.
> 
> This patch ensures "lock.release()" execution at the end of
> "storeclean()" by moving it into "finally" clause.
> 
> This patch chooses moving almost all lines in "storeclean()" into
> "_storeclean()" instead of indenting them for "try/finally" clauses,
> to keep diff simple for review-ability.
> 
> diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
> --- a/mercurial/subrepo.py
> +++ b/mercurial/subrepo.py
> @@ -521,8 +521,14 @@ class hgsubrepo(abstractsubrepo):
>          self._initrepo(r, state[0], create)
>  
>      def storeclean(self, path):
> +        lock = self._repo.lock()
> +        try:
> +            return self._storeclean(path)
> +        finally:
> +            lock.release()
> +
> +    def _storeclean(self, path):
>          clean = True
> -        lock = self._repo.lock()
>          itercache = self._calcstorehash(path)
>          try:
>              for filehash in self._readstorehashcache(path):
> @@ -539,7 +545,6 @@ class hgsubrepo(abstractsubrepo):
>                  clean = False
>              except StopIteration:
>                  pass
> -        lock.release()
>          return clean
>  
>      def _calcstorehash(self, remotepath):
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
> 


----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp

Patch

diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
--- a/mercurial/subrepo.py
+++ b/mercurial/subrepo.py
@@ -521,8 +521,14 @@  class hgsubrepo(abstractsubrepo):
         self._initrepo(r, state[0], create)
 
     def storeclean(self, path):
+        lock = self._repo.lock()
+        try:
+            return self._storeclean(path)
+        finally:
+            lock.release()
+
+    def _storeclean(self, path):
         clean = True
-        lock = self._repo.lock()
         itercache = self._calcstorehash(path)
         try:
             for filehash in self._readstorehashcache(path):
@@ -539,7 +545,6 @@  class hgsubrepo(abstractsubrepo):
                 clean = False
             except StopIteration:
                 pass
-        lock.release()
         return clean
 
     def _calcstorehash(self, remotepath):