[Freeipa-devel] [patch 0011] Temporary workaround for [patch 0010] Python list comprehension leak breaking the test execution

[Martin Basti]

On 08/17/2015 09:53 AM, Milan Kubík wrote:
> On 08/11/2015 03:23 PM, Milan Kubík wrote:
>> 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
>>
> Hi list,
>
> since I wasn't successful at solving this problem, I revert to the 
> original plan
> and propose to take the otp import test out of the execution. The 
> attached patch
> does this.
>
> By no means is this a solution to the problem. Therefore, I think the 
> ticket should remain open.
> The priority of blocker can be dropped, though. I think 'critical' 
> should be used.
>
> Cheers,
> Milan
>
>
>
ACK

Pushed to:
master: d8b9125895758cbc33821c176f3e5a05654dbee4
ipa-4-2: 57b07070f0b16f7e0099282d6a78f22c6af00793

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150820/3ad33083/attachment.htm>