[Freeipa-devel] [patch 0010] Python list comprehension leak breaking the test execution

[Christian Heimes]

On 2015-08-11 09:46, Milan Kubík wrote:
> On 08/11/2015 09:08 AM, Jan Cholasta wrote:
>> On 11.8.2015 09:00, Milan Kubík wrote:
>>
>>> On 08/10/2015 06:22 PM, Milan Kubík wrote:
>>>
>>>> On 08/10/2015 06:02 PM, Milan Kubík wrote:
>>>>
>>>>> On 08/10/2015 05:54 PM, Jan Cholasta wrote:
>>>>>
>>>>>> On 10.8.2015 17:43, Milan Kubík wrote:
>>>>>>
>>>>>>> Hi all,
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> this patch fixes problem described in the ticket [1]
>>>>>>>
>>>>>>> that caused the test run to fail completely at every other or so
>>>>>>> run.
>>>>>>>
>>>>>>> I took the liberty to fix most of the pep8 issues while I was at it.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Thanks to Jan Cholasta for help with identifying this one.
>>>>>>>
>>>>>>
>>>>>>
>>>>>> IMO this would be more robust:
>>>>>>
>>>>>>
>>>>>>
>>>>>>     t = None
>>>>>>
>>>>>>     try:
>>>>>>
>>>>>>         ...
>>>>>>
>>>>>>     finally:
>>>>>>
>>>>>>         del t
>>>>>>
>>>>>>         nss.nss_shutdown()
>>>>>>
>>>>>>
>>>>>>
>>>>>> By assigning a value to the variable at the beginning you make sure
>>>>>>
>>>>>> that the del statement will not fail.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Honza
>>>>>>
>>>>>>
>>>>>>
>>>>> Thanks for the idea. It also removed the version check.
>>>>>
>>>>> Updated patch attached.
>>>>>
>>>>>
>>>>>
>>>>> Milan
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>> Self NACK.
>>>>
>>>>
>>>>
>>>> I have updated the fix for all of the leaks in that class. It seems to
>>>>
>>>> corrupt the database even when no init/shutdown was called. Just not
>>>>
>>>> so often.
>>>>
>>>>
>>>>
>>>> Milan
>>>>
>>>>
>>>>
>>>>
>>>>
>>> NACK again. The problem is the reference counting. Even with this patch,
>>>
>>> there seems to be at least one reference left after 't' is deleted and
>>>
>>> nss.nss_shutdown races with the garbage collector.
>>>
>>
>>
>> Doesn't the PKCDDocument object also reference some NSS objects? It
>> might be worth trying to delete it manually before nss_shutdown as well.
>>
>>
>>
>>
>>
> Yes, this patch doesn't work. There are still some references left.
> 
> The problem may be on multiple places in here [1].
> 
> There may be a reference still bound to the doc label.
> Another problem is that python 2 code:
> 
> [x for x in [123, 456]]
> 
> creates 2 references to 456 as the list used in the assert lives for
> some time
> before it is garbage collected even though it is not reachable,
> holding a reference to the object labeled as t.

In Python 2 the list comprehension leaks the internal loop variable. It
was an oversight in the design of list comprehensions. This has been
fixed in Python 3. You can work around the problem with a generator
expression inside list()

   list(x for x in [123, 456])

is equivalent to

  [x for x in [123, 456]]

except it doesn't leak x to the outer scope.

If you suspect an issue related to cyclic GC, you can force a garbage
collector run with gc.collect().

Christian


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 455 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150811/8ca45b75/attachment.sig>