Patchwork localrepo: prevent leak of transaction object (issue4258)

login
register
mail settings
Submitter Gregory Szorc
Date May 22, 2014, 1:02 a.m.
Message ID <86829970c3fbf3134560.1400720578@vm-ubuntu-main.gateway.sonic.net>
Download mbox | patch
Permalink /patch/4847/
State Accepted
Commit 99ba1d082287e5d701c86dfea7e7a5f9adc9dad4
Headers show

Comments

Gregory Szorc - May 22, 2014, 1:02 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1400720555 25200
#      Wed May 21 18:02:35 2014 -0700
# Node ID 86829970c3fbf3134560a143f74444066d2e9623
# Parent  9fb6f328576ac4e38f4e5071c4d669a6ceb3a76e
localrepo: prevent leak of transaction object (issue4258)

The onclose() closure added in cd443c7589cc held a regular reference to
the transaction object. This was causing the transaction to not gc and
a leak to occur.

The closure now holds a reference to the weakref instance and the leak
goes away.
Durham Goode - May 22, 2014, 1:36 a.m.
On 5/21/14, 6:02 PM, "Gregory Szorc" <gregory.szorc@gmail.com> wrote:

># HG changeset patch

># User Gregory Szorc <gregory.szorc@gmail.com>

># Date 1400720555 25200

>#      Wed May 21 18:02:35 2014 -0700

># Node ID 86829970c3fbf3134560a143f74444066d2e9623

># Parent  9fb6f328576ac4e38f4e5071c4d669a6ceb3a76e

>localrepo: prevent leak of transaction object (issue4258)

>

>The onclose() closure added in cd443c7589cc held a regular reference to

>the transaction object. This was causing the transaction to not gc and

>a leak to occur.

>

>The closure now holds a reference to the weakref instance and the leak

>goes away.

>

>diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py

>--- a/mercurial/localrepo.py

>+++ b/mercurial/localrepo.py

>@@ -860,9 +860,9 @@ class localrepository(object):

>                 _("abandoned transaction found"),

>                 hint=_("run 'hg recover' to clean up transaction"))

> 

>         def onclose():

>-            self.store.write(tr)

>+            self.store.write(self._transref())

> 

>         self._writejournal(desc)

>         renames = [(vfs, x, undoname(x)) for vfs, x in

>self._journalfiles()]

>         rp = report and report or self.ui.warn


I’m not familiar with the weak reference stuff in transactions, but if the
tests pass, this looks good to me.  Should probably be queued for STABLE
though.

Thanks for fixing this.
Augie Fackler - May 22, 2014, 3:11 p.m.
On May 21, 2014, at 9:36 PM, Durham Goode <durham@fb.com> wrote:

> On 5/21/14, 6:02 PM, "Gregory Szorc" <gregory.szorc@gmail.com> wrote:
> 
>> # HG changeset patch
>> # User Gregory Szorc <gregory.szorc@gmail.com>
>> # Date 1400720555 25200
>> #      Wed May 21 18:02:35 2014 -0700
>> # Node ID 86829970c3fbf3134560a143f74444066d2e9623
>> # Parent  9fb6f328576ac4e38f4e5071c4d669a6ceb3a76e
>> localrepo: prevent leak of transaction object (issue4258)
>> 
>> The onclose() closure added in cd443c7589cc held a regular reference to
>> the transaction object. This was causing the transaction to not gc and
>> a leak to occur.
>> 
>> The closure now holds a reference to the weakref instance and the leak
>> goes away.
>> 
>> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>> --- a/mercurial/localrepo.py
>> +++ b/mercurial/localrepo.py
>> @@ -860,9 +860,9 @@ class localrepository(object):
>>                _("abandoned transaction found"),
>>                hint=_("run 'hg recover' to clean up transaction"))
>> 
>>        def onclose():
>> -            self.store.write(tr)
>> +            self.store.write(self._transref())
>> 
>>        self._writejournal(desc)
>>        renames = [(vfs, x, undoname(x)) for vfs, x in
>> self._journalfiles()]
>>        rp = report and report or self.ui.warn
> 
> I’m not familiar with the weak reference stuff in transactions, but if the
> tests pass, this looks good to me.  Should probably be queued for STABLE
> though.

Agreed. I'm going to defer to mpm on this one because I'm not familiar enough with weakrefs.

> 
> Thanks for fixing this.
> 
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Matt Mackall - May 30, 2014, 6:50 p.m.
On Wed, 2014-05-21 at 18:02 -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1400720555 25200
> #      Wed May 21 18:02:35 2014 -0700
> # Node ID 86829970c3fbf3134560a143f74444066d2e9623
> # Parent  9fb6f328576ac4e38f4e5071c4d669a6ceb3a76e
> localrepo: prevent leak of transaction object (issue4258)

Queued for stable, thanks.

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -860,9 +860,9 @@  class localrepository(object):
                 _("abandoned transaction found"),
                 hint=_("run 'hg recover' to clean up transaction"))
 
         def onclose():
-            self.store.write(tr)
+            self.store.write(self._transref())
 
         self._writejournal(desc)
         renames = [(vfs, x, undoname(x)) for vfs, x in self._journalfiles()]
         rp = report and report or self.ui.warn