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

[Milan Kubík]

On 08/11/2015 09:53 AM, Jan Cholasta wrote:
> 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 ow I think
>> 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)
>
It seems that NSS decided the assumption doesn't hold. [1]
Plus, the decorator seems to change the execution order of the test cases.

[1]: https://paste.fedoraproject.org/253846/39299083/

Milan