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

[Maciej Fijalkowski]

class A(object):
    def __del__(self):
        print "del"

class B(object):
    pass

b = B()
b.b = b
b.a = A()


This example does not call __del__ in CPython either.

The __del__ is not guaranteed to be called - that's why there is a
painful module finalization procedure where CPython is trying to call
"as much as possible", but there are still no guarantees. If you add
del b; gc.collect() you will see "del" printed. Of course this
involves a cycle, but cycles can come in ways that you don't expect
them and PyPy simply says "everything is GCed". I think it's very much
in line with what python-dev thinks.

On Tue, Apr 5, 2016 at 2:21 AM, Pierre-Yves David
<pierre-yves.david at ens-lyon.org> wrote:
> 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
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel