Patchwork tr-summary: keep a weakref to the unfiltered repository

login
register
mail settings
Submitter Boris Feld
Date Nov. 25, 2017, 9:35 p.m.
Message ID <a2eccde70ce65f9f0e87.1511645751@FB>
Download mbox | patch
Permalink /patch/25752/
State Accepted
Headers show

Comments

Boris Feld - Nov. 25, 2017, 9:35 p.m.
# HG changeset patch
# User Boris Feld <boris.feld@octobus.net>
# Date 1511578301 18000
#      Fri Nov 24 21:51:41 2017 -0500
# Branch stable
# Node ID a2eccde70ce65f9f0e8735e1bc8d51f58c8cdd71
# Parent  02845f7441aff30bc01975a5881cabfa922c12d4
# EXP-Topic callback-error
# Available At https://bitbucket.org/octobus/mercurial-devel/
#              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r a2eccde70ce6
tr-summary: keep a weakref to the unfiltered repository

Repoview can have a different life cycle, causing issue in some corner
cases. The particular instance that revealed this comes from localpeer. The
localpeer hold a reference to the unfiltered repository, but calling 'local()'
will create an on-demand 'visible' repoview. That repoview can be garbaged
collected any time. Here is a simplified step by step reproduction::

    1) tr = peer.local().transaction('foo')
    2) tr.close()

After (1), the repoview object is garbage collected, so weakref used in (2)
point to nothing.


Thanks to Sean Farley for helping raising and debugging this issue.
Boris Feld - Nov. 25, 2017, 9:39 p.m.
Sorry that was meant for stable but I forget the flag, should I resend
it?

On Sat, 2017-11-25 at 16:35 -0500, Boris Feld wrote:
> # HG changeset patch
> # User Boris Feld <boris.feld@octobus.net>
> # Date 1511578301 18000
> #      Fri Nov 24 21:51:41 2017 -0500
> # Branch stable
> # Node ID a2eccde70ce65f9f0e8735e1bc8d51f58c8cdd71
> # Parent  02845f7441aff30bc01975a5881cabfa922c12d4
> # EXP-Topic callback-error
> # Available At https://bitbucket.org/octobus/mercurial-devel/
> #              hg pull https://bitbucket.org/octobus/mercurial-devel/
>  -r a2eccde70ce6
> tr-summary: keep a weakref to the unfiltered repository
> 
> Repoview can have a different life cycle, causing issue in some
> corner
> cases. The particular instance that revealed this comes from
> localpeer. The
> localpeer hold a reference to the unfiltered repository, but calling
> 'local()'
> will create an on-demand 'visible' repoview. That repoview can be
> garbaged
> collected any time. Here is a simplified step by step reproduction::
> 
>     1) tr = peer.local().transaction('foo')
>     2) tr.close()
> 
> After (1), the repoview object is garbage collected, so weakref used
> in (2)
> point to nothing.
> 
> 
> Thanks to Sean Farley for helping raising and debugging this issue.
> 
> diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
> --- a/mercurial/scmutil.py
> +++ b/mercurial/scmutil.py
> @@ -1233,9 +1233,17 @@ def registersummarycallback(repo, otr, t
>  
>      def reportsummary(func):
>          """decorator for report callbacks."""
> -        reporef = weakref.ref(repo)
> +        # The repoview life cycle is shorter than the one of the
> actual
> +        # underlying repository. So the filtered object can die
> before the
> +        # weakref is used leading to troubles. We keep a reference
> to the
> +        # unfiltered object and restore the filtering when
> retrieving the
> +        # repository through the weakref.
> +        filtername = repo.filtername
> +        reporef = weakref.ref(repo.unfiltered())
>          def wrapped(tr):
>              repo = reporef()
> +            if filtername:
> +                repo = repo.filtered(filtername)
>              func(repo, tr)
>          newcat = '%2i-txnreport' % len(categories)
>          otr.addpostclose(newcat, wrapped)
Yuya Nishihara - Nov. 27, 2017, 12:17 p.m.
On Sat, 25 Nov 2017 16:39:32 -0500, Boris Feld wrote:
> Sorry that was meant for stable but I forget the flag, should I resend
> it?
> 
> On Sat, 2017-11-25 at 16:35 -0500, Boris Feld wrote:
> > # HG changeset patch
> > # User Boris Feld <boris.feld@octobus.net>
> > # Date 1511578301 18000
> > #      Fri Nov 24 21:51:41 2017 -0500
> > # Branch stable
> > # Node ID a2eccde70ce65f9f0e8735e1bc8d51f58c8cdd71
> > # Parent  02845f7441aff30bc01975a5881cabfa922c12d4
> > # EXP-Topic callback-error
> > # Available At https://bitbucket.org/octobus/mercurial-devel/
> > #              hg pull https://bitbucket.org/octobus/mercurial-devel/
> >  -r a2eccde70ce6
> > tr-summary: keep a weakref to the unfiltered repository

Queued for stable, thanks.

FWIW, I think the transaction object should hold the current repo object
instead of doing lots of weakref business.

Patch

diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -1233,9 +1233,17 @@  def registersummarycallback(repo, otr, t
 
     def reportsummary(func):
         """decorator for report callbacks."""
-        reporef = weakref.ref(repo)
+        # The repoview life cycle is shorter than the one of the actual
+        # underlying repository. So the filtered object can die before the
+        # weakref is used leading to troubles. We keep a reference to the
+        # unfiltered object and restore the filtering when retrieving the
+        # repository through the weakref.
+        filtername = repo.filtername
+        reporef = weakref.ref(repo.unfiltered())
         def wrapped(tr):
             repo = reporef()
+            if filtername:
+                repo = repo.filtered(filtername)
             func(repo, tr)
         newcat = '%2i-txnreport' % len(categories)
         otr.addpostclose(newcat, wrapped)