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

2015-08-19 Thread Martin Kosek
On 08/18/2015 07:21 PM, Alexander Bokovoy wrote:
 On Mon, 17 Aug 2015, Nathaniel McCallum wrote:
 On Mon, 2015-08-10 at 17:43 +0200, 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.

 [1]: https://fedorahosted.org/freeipa/ticket/5192

 I think the right answer is to move this to python-cryptography.

 We already have python-cryptography as a dependency. And in this case
 the crypto code is pretty well self-contained. Aside from nss database
 initialization, nss is only used in:
 * convertAlgorithm() (constants only)
 * XMLDecryptor (actual decryption code)

 The migration should be straightforward. It is probably a 1 day task. I
 could probably tackle it later this week.

 Should we do this?
 I think we should -- for FreeIPA 4.3 or 4.4.

That would we awesome Nathaniel! I remember too many bugs related to python-nss
initialization and context leaks - if we get rid of those one for all, we are
golden.

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


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

2015-08-18 Thread Alexander Bokovoy

On Mon, 17 Aug 2015, Nathaniel McCallum wrote:

On Mon, 2015-08-10 at 17:43 +0200, 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.

[1]: https://fedorahosted.org/freeipa/ticket/5192


I think the right answer is to move this to python-cryptography.

We already have python-cryptography as a dependency. And in this case
the crypto code is pretty well self-contained. Aside from nss database
initialization, nss is only used in:
* convertAlgorithm() (constants only)
* XMLDecryptor (actual decryption code)

The migration should be straightforward. It is probably a 1 day task. I
could probably tackle it later this week.

Should we do this?

I think we should -- for FreeIPA 4.3 or 4.4.
--
/ Alexander Bokovoy

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


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

2015-08-17 Thread Nathaniel McCallum
On Mon, 2015-08-10 at 17:43 +0200, 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.
 
 [1]: https://fedorahosted.org/freeipa/ticket/5192

I think the right answer is to move this to python-cryptography.

We already have python-cryptography as a dependency. And in this case
the crypto code is pretty well self-contained. Aside from nss database
initialization, nss is only used in:
* convertAlgorithm() (constants only)
* XMLDecryptor (actual decryption code)

The migration should be straightforward. It is probably a 1 day task. I
could probably tackle it later this week.

Should we do this?

Nathaniel

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2015-08-11 Thread Milan Kubík

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/

Milan

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2015-08-11 Thread Martin Babinsky

On 08/10/2015 05:43 PM, 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.

[1]: https://fedorahosted.org/freeipa/ticket/5192

Cheers,
Milan



Hi Milan,

I did not follow the rest of the conversation (probably should), but you 
can also workaround this python2 feature by using:



assert list((t.id, t.options) for t in doc.getKeyPackages()) == \


In this case a list is constructed by calling a generator expression 
which does not leak variables to the outside scope.


It may or may not help you but I think it's a trick worth trying. As a 
bonus it should be py2/3 compatible.


--
Martin^3 Babinsky

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


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

2015-08-11 Thread Jan Cholasta

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.


--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


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

2015-08-11 Thread 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

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2015-08-11 Thread Milan Kubík

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.
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2015-08-11 Thread Milan Kubík

On 08/11/2015 12:38 PM, Jan Cholasta wrote:

On 11.8.2015 12:33, Milan Kubík wrote:

On 08/11/2015 12:33 PM, Milan Kubík wrote:

On 08/11/2015 10:03 AM, Milan Kubík wrote:

On 08/11/2015 09:57 AM, Martin Babinsky wrote:

On 08/10/2015 05:43 PM, 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.

[1]: https://fedorahosted.org/freeipa/ticket/5192

Cheers,
Milan



Hi Milan,

I did not follow the rest of the conversation (probably should), but
you can also workaround this python2 feature by using:


assert list((t.id, t.options) for t in doc.getKeyPackages()) == \


In this case a list is constructed by calling a generator expression
which does not leak variables to the outside scope.

It may or may not help you but I think it's a trick worth trying. As
a bonus it should be py2/3 compatible.


Thanks for the suggestion, Martin.

I will try that. Though, I suspect there may be other labels holding
references to nss objects. I think I will combine this with Jan's
suggestion.

I will send an update once I'll have the results from a test run.

Milan


After running the test with the decorator and list comprehensions
replaced by a list initialization from a generator expression, it
fails on this [1].

Does anyone know what is this teardown method supposed to do?

Milan

[1]: https://paste.fedoraproject.org/253792/14392890/



The error message means that the NSS database directory does not 
exist, it is not readable, or the NSS database is not initialized (as 
in certutil -N).


Thanks. My main question is why is it called in the teardown method (btw 
remnant from nose times it seems, but pytest executes it after every 
test case)


Milan

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2015-08-11 Thread Milan Kubík

On 08/11/2015 10:03 AM, Milan Kubík wrote:

On 08/11/2015 09:57 AM, Martin Babinsky wrote:

On 08/10/2015 05:43 PM, 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.

[1]: https://fedorahosted.org/freeipa/ticket/5192

Cheers,
Milan



Hi Milan,

I did not follow the rest of the conversation (probably should), but 
you can also workaround this python2 feature by using:



assert list((t.id, t.options) for t in doc.getKeyPackages()) == \


In this case a list is constructed by calling a generator expression 
which does not leak variables to the outside scope.


It may or may not help you but I think it's a trick worth trying. As 
a bonus it should be py2/3 compatible.



Thanks for the suggestion, Martin.

I will try that. Though, I suspect there may be other labels holding 
references to nss objects. I think I will combine this with Jan's 
suggestion.


I will send an update once I'll have the results from a test run.

Milan

After running the test with the decorator and list comprehensions 
replaced by a list initialization from a generator expression, it fails 
on this [1].


Does anyone know what is this teardown method supposed to do?

Milan

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2015-08-11 Thread Milan Kubík

On 08/11/2015 12:33 PM, Milan Kubík wrote:

On 08/11/2015 10:03 AM, Milan Kubík wrote:

On 08/11/2015 09:57 AM, Martin Babinsky wrote:

On 08/10/2015 05:43 PM, 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.

[1]: https://fedorahosted.org/freeipa/ticket/5192

Cheers,
Milan



Hi Milan,

I did not follow the rest of the conversation (probably should), but 
you can also workaround this python2 feature by using:



assert list((t.id, t.options) for t in doc.getKeyPackages()) == \


In this case a list is constructed by calling a generator expression 
which does not leak variables to the outside scope.


It may or may not help you but I think it's a trick worth trying. As 
a bonus it should be py2/3 compatible.



Thanks for the suggestion, Martin.

I will try that. Though, I suspect there may be other labels holding 
references to nss objects. I think I will combine this with Jan's 
suggestion.


I will send an update once I'll have the results from a test run.

Milan

After running the test with the decorator and list comprehensions 
replaced by a list initialization from a generator expression, it 
fails on this [1].


Does anyone know what is this teardown method supposed to do?

Milan

[1]: https://paste.fedoraproject.org/253792/14392890/

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2015-08-11 Thread 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




signature.asc
Description: OpenPGP digital signature
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2015-08-11 Thread Milan Kubík

On 08/11/2015 01:46 PM, Christian Heimes wrote:

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


Hi Christian, I already use the generator expression as Martin suggested 
earlier today. Together with the decorator proposed by Jan. I had some 
trouble with the test teardown method, but it seems I had a minor 
problem in my environment as there was missing ~/.ipa/alias directory in 
the test run.


On the other hand, while not changing the job on jenkins I wonder why it 
worked sometimes in this env.


Milan

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2015-08-11 Thread Jan Cholasta

On 11.8.2015 12:33, Milan Kubík wrote:

On 08/11/2015 12:33 PM, Milan Kubík wrote:

On 08/11/2015 10:03 AM, Milan Kubík wrote:

On 08/11/2015 09:57 AM, Martin Babinsky wrote:

On 08/10/2015 05:43 PM, 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.

[1]: https://fedorahosted.org/freeipa/ticket/5192

Cheers,
Milan



Hi Milan,

I did not follow the rest of the conversation (probably should), but
you can also workaround this python2 feature by using:


assert list((t.id, t.options) for t in doc.getKeyPackages()) == \


In this case a list is constructed by calling a generator expression
which does not leak variables to the outside scope.

It may or may not help you but I think it's a trick worth trying. As
a bonus it should be py2/3 compatible.


Thanks for the suggestion, Martin.

I will try that. Though, I suspect there may be other labels holding
references to nss objects. I think I will combine this with Jan's
suggestion.

I will send an update once I'll have the results from a test run.

Milan


After running the test with the decorator and list comprehensions
replaced by a list initialization from a generator expression, it
fails on this [1].

Does anyone know what is this teardown method supposed to do?

Milan

[1]: https://paste.fedoraproject.org/253792/14392890/



The error message means that the NSS database directory does not exist, 
it is not readable, or the NSS database is not initialized (as in 
certutil -N).


--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2015-08-11 Thread 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

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2015-08-10 Thread Milan Kubík

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
From d0f758f28a657db84b0c0f8ab9631c6519522492 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Milan=20Kub=C3=ADk?= mku...@redhat.com
Date: Mon, 10 Aug 2015 16:16:54 +0200
Subject: [PATCH] tests: fix the list comprehension leak in python 2 and free
 all nss objects

The python 2 feature/bug that binds the last value in a list
comprehension to the variable caused the nss object in the test
being bound at the time when nss.nss_shutdown was called.

This caused an ```NSPRError: (SEC_ERROR_BUSY) NSS could not shutdown.
Objects are still in use.``` exception being thrown that was not caught.
The subsequent calls to nss module corrupted the NSS database and this
led to a cascade of another errors invalidating the whole test run.

The patch works around this python 2 issue.

Fixes: https://fedorahosted.org/freeipa/ticket/5192
---
 ipatests/test_ipaserver/test_otptoken_import.py | 23 +--
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/ipatests/test_ipaserver/test_otptoken_import.py b/ipatests/test_ipaserver/test_otptoken_import.py
index c8818a01ea4d4bad4b4a0f656357f7b32407b86a..1d572d1b7c47e72c4c3c0f7ef6bf8443b02533cc 100644
--- a/ipatests/test_ipaserver/test_otptoken_import.py
+++ b/ipatests/test_ipaserver/test_otptoken_import.py
@@ -19,7 +19,6 @@
 
 import os
 import sys
-import nose
 from nss import nss
 from ipalib.x509 import initialize_nss_database
 
@@ -27,6 +26,7 @@ from ipaserver.install.ipa_otptoken_import import PSKCDocument, ValidationError
 
 basename = os.path.join(os.path.dirname(__file__), data)
 
+
 class test_otptoken_import(object):
 
 def teardown(self):
@@ -50,7 +50,7 @@ class test_otptoken_import(object):
 assert doc.keyname is None
 try:
 [(t.id, t.options) for t in doc.getKeyPackages()]
-except ValidationError: # Referenced keys are not supported.
+except ValidationError:  # Referenced keys are not supported.
 pass
 else:
 assert False
@@ -60,13 +60,14 @@ class test_otptoken_import(object):
 assert doc.keyname is None
 try:
 [(t.id, t.options) for t in doc.getKeyPackages()]
-except ValidationError: # PIN Policy is not supported.
+except ValidationError:  # PIN Policy is not supported.
 pass
 else:
 assert False
 
 def test_figure6(self):
 nss.nss_init_nodb()
+t = None
 try:
 doc = PSKCDocument(os.path.join(basename, pskc-figure6.xml))
 assert doc.keyname == 'Pre-shared-key'
@@ -80,10 +81,12 @@ class test_otptoken_import(object):
 'ipatokenotpdigits': 8,
 'type': u'hotp'})]
 finally:
+del t
 nss.nss_shutdown()
 
 def test_figure7(self):
 nss.nss_init_nodb()
+t = None
 try:
 doc = PSKCDocument(os.path.join(basename, pskc-figure7.xml))
 assert doc.keyname == 'My Password 1'
@@ -95,14 +98,16 @@ class test_otptoken_import(object):
 'ipatokenserial': u'987654321',
 'ipatokenotpdigits': 8,
 'type': u'hotp'})]
+
 finally:
+del t
 nss.nss_shutdown()
 
 def test_figure8(self):
 nss.nss_init_nodb()
 try:
 doc = PSKCDocument(os.path.join(basename, pskc-figure8.xml))
-except NotImplementedError: # X.509 is not supported.
+except NotImplementedError:  # X.509 is not supported.
 pass
 else:
 assert False
@@ -113,7 +118,7 @@ class test_otptoken_import(object):
 nss.nss_init_nodb()
 try:
 doc = PSKCDocument(os.path.join(basename, pskc-invalid.xml))
-except ValueError: # File is invalid.
+except ValueError:  # File is invalid.
 pass
 else:
 assert False
@@ -122,18 +127,22 @@ class test_otptoken_import(object):
 
 def test_mini(self):
 nss.nss_init_nodb()
+t = None
 try:
 doc = PSKCDocument(os.path.join(basename, pskc-mini.xml))
 [(t.id, t.options) for t in doc.getKeyPackages()]
-except ValidationError: # Unsupported token type.
+
+except ValidationError:  # Unsupported token type.
   

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

2015-08-10 Thread Milan Kubík

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
From 9801778e5d890d099a4be3fa66e85cdf89f31084 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Milan=20Kub=C3=ADk?= mku...@redhat.com
Date: Mon, 10 Aug 2015 16:16:54 +0200
Subject: [PATCH] tests: fix the list comprehension leak in python 2 and free
 all nss objects

The python 2 feature/bug that binds the last value in a list
comprehension to the variable caused the nss object in the test
being bound at the time when nss.nss_shutdown was called.

This caused an ```NSPRError: (SEC_ERROR_BUSY) NSS could not shutdown.
Objects are still in use.``` exception being thrown that was not caught.
The subsequent calls to nss module corrupted the NSS database and this
led to a cascade of another errors invalidating the whole test run.

The patch works around this python 2 issue.

Fixes: https://fedorahosted.org/freeipa/ticket/5192
---
 ipatests/test_ipaserver/test_otptoken_import.py | 31 -
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/ipatests/test_ipaserver/test_otptoken_import.py b/ipatests/test_ipaserver/test_otptoken_import.py
index c8818a01ea4d4bad4b4a0f656357f7b32407b86a..6f383ea7a3f511733aacef20eee54440bd513ec6 100644
--- a/ipatests/test_ipaserver/test_otptoken_import.py
+++ b/ipatests/test_ipaserver/test_otptoken_import.py
@@ -19,7 +19,6 @@
 
 import os
 import sys
-import nose
 from nss import nss
 from ipalib.x509 import initialize_nss_database
 
@@ -27,6 +26,7 @@ from ipaserver.install.ipa_otptoken_import import PSKCDocument, ValidationError
 
 basename = os.path.join(os.path.dirname(__file__), data)
 
+
 class test_otptoken_import(object):
 
 def teardown(self):
@@ -34,6 +34,7 @@ class test_otptoken_import(object):
 
 def test_figure3(self):
 doc = PSKCDocument(os.path.join(basename, pskc-figure3.xml))
+t = None
 assert doc.keyname is None
 assert [(t.id, t.options) for t in doc.getKeyPackages()] == \
 [(u'12345678', {
@@ -44,29 +45,37 @@ class test_otptoken_import(object):
 'ipatokenotpdigits': 8,
 'type': u'hotp',
 })]
+del t
 
 def test_figure4(self):
 doc = PSKCDocument(os.path.join(basename, pskc-figure4.xml))
+t = None
 assert doc.keyname is None
 try:
 [(t.id, t.options) for t in doc.getKeyPackages()]
-except ValidationError: # Referenced keys are not supported.
+except ValidationError:  # Referenced keys are not supported.
 pass
 else:
 assert False
+finally:
+del t
 
 def test_figure5(self):
 doc = PSKCDocument(os.path.join(basename, pskc-figure5.xml))
+t = None
 assert doc.keyname is None
 try:
 [(t.id, t.options) for t in doc.getKeyPackages()]
-except ValidationError: # PIN Policy is not supported.
+except ValidationError:  # PIN Policy is not supported.
 pass
 else:
 assert False
+finally:
+del t
 
 def test_figure6(self):
 nss.nss_init_nodb()
+t = None
 try:
 doc = PSKCDocument(os.path.join(basename, pskc-figure6.xml))
 assert doc.keyname == 'Pre-shared-key'
@@ -80,10 +89,12 @@ class test_otptoken_import(object):
 'ipatokenotpdigits': 8,
 'type': u'hotp'})]
 finally:
+del t
 nss.nss_shutdown()
 
 def test_figure7(self):
 nss.nss_init_nodb()
+t = None
 try:
 doc = PSKCDocument(os.path.join(basename, pskc-figure7.xml))
 assert doc.keyname == 'My Password 1'
@@ -95,14 +106,16 @@ class test_otptoken_import(object):
 'ipatokenserial': u'987654321',
 'ipatokenotpdigits': 8,
 'type': u'hotp'})]
+
 finally:
+del t
 nss.nss_shutdown()
 
 def test_figure8(self):
 nss.nss_init_nodb()
 try:
 doc = PSKCDocument(os.path.join(basename, pskc-figure8.xml))
-except 

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

2015-08-10 Thread Jan Cholasta

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

--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code