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

[Gregory Szorc]

On Fri, Apr 1, 2016 at 2:13 AM, Pierre-Yves David <
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>
>> wrote:
>>
>>>
>>>
>>> On Mar 31, 2016, at 23:58, Maciej Fijalkowski <fijall at gmail.com> wrote:
>>>>
>>>> # HG changeset patch
>>>> # User Maciej Fijalkowski <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?

We could establish a variable holding a set of lock instances. When
lock.release is called, the instance is removed from the set. After command
dispatch or during process exit, we could see if any locks weren't released
and issue warnings. The set of lock instances would need to be a global
variable because it can't be a member of e.g. repo instances because then
you'd need __del__ or a similar "leak checking" mechanism for repos. But
once you make it a global variable you have problems with e.g. multiple
threads. So you need to key locks to a thread/dispatch ID or only warn on
process exit. Yuck.

I'm tempted to say that the lock accounting should be implemented as an
extension that is always loaded when running tests. This gets us our
warning without making the run-time behavior too complicated.

Furthermore, I'd consider moving the logic from lock.release() into
lock._release() and have lock.release() issue a developer warning unless
called with a flag saying to suppress it. Most callers should be using
locks as context managers now. Context managers prevent bugs and we should
be prodding all consumers to use locks as context managers and explicitly
drawing attention to call sites that don't (by requiring a flag to suppress
the warning).
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20160403/b113918e/attachment.html>