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

[Jan Cholasta]

On 11.8.2015 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.
>
> I don't know how nss counts the references to its objects but I think we
> should
> delete all the objects that use/are used by nss explicitly. This means
> assigning the list
> produced by the list comprehension a name as well and the delete it when
> it is not needed.
>
> I'll send the patch shortly.
>
> [1]: https://paste.fedoraproject.org/253748/92783071/

Given an assumption that all objects referenced only by local variables 
are deleted when a function returns, maybe this can be solved with:

     def nss_initialized(func):
         def decorated(*args, **kwargs):
             nss.nss_init_nodb()
             try:
                 func(*args, **kwargs)
             finally:
                 nss.nss_shutdown()
         return decorated

     ...

     class test_class(...):
         @nss_initialized
         def test_method(self):
             ...

(OTOH and untested)

-- 
Jan Cholasta