[Reviewers] [PATCH rfc] rfc: call gc at exit of mercurial

[Pierre-Yves David]

TL;DR; committer, pypy skip garbage collection at process exit, not call 
__del__ in this case. This seems a divergence from cpython standard. Are 
we okay with that?

More details below.

On 04/03/2016 10:51 AM, Gregory Szorc wrote:
> On Fri, Apr 1, 2016 at 2:13 AM, Pierre-Yves David
> <pierre-yves.david at ens-lyon.org <mailto:pierre-yves.david at ens-lyon.org>>
> wrote:
>
>
>
>     On 04/01/2016 12:10 AM, Maciej Fijalkowski wrote:
>
>         On Fri, Apr 1, 2016 at 9:09 AM, Gregory Szorc
>         <gregory.szorc at gmail.com <mailto:gregory.szorc at gmail.com>> wrote:
>
>
>
>                 On Mar 31, 2016, at 23:58, Maciej Fijalkowski
>                 <fijall at gmail.com <mailto:fijall at gmail.com>> wrote:
>
>                 # HG changeset patch
>                 # User Maciej Fijalkowski <fijall at gmail.com
>                 <mailto:fijall at gmail.com>>
>                 # Date 1459493914 -7200
>                 #      Fri Apr 01 08:58:34 2016 +0200
>                 # Node ID 66b3737b62635691b5a205dafc80e640880b77ca
>                 # Parent  9ca7854b963295b8b56d276b903623ac8277f18d
>                 rfc: call gc at exit of mercurial
>
>                 This is not a proper patch, just a visualization of a
>                 hack I did.
>                 Locks have a __del__, which test-devel-warnings rely on
>                 being
>                 called at the exit of a program. Locks also have
>                 __exit__ which is
>                 a more proper way to handle locks, but that test does
>                 not respect it.
>                 I would like to know what's the proper way of handling
>                 this - should
>                 I fix test, should I just call the GC (which will make
>                 the command
>                 line tool slower) or what's the proposed solution
>
>
>             __del__ should be purged and banned from the code base
>             because it undermines GC and can lead to memory leaks.
>
>             For holding on to temporary resources, context managers
>             (preferred) or try..finally should be used.
>
>
>         So what do we do with lock.__del__ and how tests rely on it?
>
>
>     Can we get details about this test failure and why it relies on it?
>
>
> The existence of lock.__del__ is to catch bugs where people forget to
> call lock.release. Unfortunately, the presence of __del__ does bad
> things for garbage collection. So, we want to have a mechanism to detect
> failure to call lock.release but it can't use __del__. What can we do?

It seems like no standard code path rely on this __del__ and it only 
exist as a double safety. Moreover, the __del__ pattern create issue for 
gc in case of cycle and we are quite careful not to create cycle in our 
low level object.

We seems to have 2 code path that trigger this safeguard in tests. On is 
the devel warning on-purpose-clownly code and the second one is an 
extensions enforcing a hard crash to test fncache recovery.


About the specific issue here:

Pypy does not automatically garbage object when reference count hit 
zero. It also skip the garbase collecting (and __del__ call) when the 
programme exit. That seems like a divergence from the documented cpython 
behavior.

As we are not supposed to rely on this behavior, we might consider that 
Mercurial will be fine with this behavior. And in practice the only code 
broken by this lack of __del__ call is one where the code is especially 
wrong on purpose. This test is not testing for the __del__ call or any 
other implicite rollback of the transaction. So we can just call it 
directly.

If we are fine with pypy not running any __del__ the following patch is 
probably all we want. I've CCed the other committers for opinions.

--- a/tests/test-devel-warnings.t
+++ b/tests/test-devel-warnings.t
@@ -9,10 +9,11 @@
    > command = cmdutil.command(cmdtable)
    >
    > @command('buggylocking', [], '')
    > def buggylocking(ui, repo):
    >     tr = repo.transaction('buggy')
+  >     tr.release() # make sure we rollback the transaction as pypy 
skip the __del__
    >     lo = repo.lock()
    >     wl = repo.wlock()
    >     wl.release()
    >     lo.release()
    >


-- 
Pierre-Yves David