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

[Milan Kubík]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mkubik-0011-ipatests-Take-otptoken-import-test-out-of-execution.patch
Type: text/x-patch
Size: 1358 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150817/68bad2ca/attachment.bin>