Re: [Freeipa-devel] [patch 0010] Python list comprehension leak breaking the test execution
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
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
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
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
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
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
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
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
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
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
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
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
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
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
[Freeipa-devel] [patch 0010] Python list comprehension leak breaking the test execution
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 From b467c5d81ff3cf54363da687132a3f205ef46d15 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 | 25 +++-- 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/ipatests/test_ipaserver/test_otptoken_import.py b/ipatests/test_ipaserver/test_otptoken_import.py index c8818a01ea4d4bad4b4a0f656357f7b32407b86a..7268ace35764df6f30036e2c17f8197bff01c6ea 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,7 +60,7 @@ 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 @@ -79,6 +79,10 @@ class test_otptoken_import(object): 'ipatokenhotpcounter': 0, 'ipatokenotpdigits': 8, 'type': u'hotp'})] +# should fix the list comprehension leak in python 2 +# python -c '[x for x in [1, 2]]; print(x)' +if sys.version_info.major == 2: +del t finally: nss.nss_shutdown() @@ -95,6 +99,9 @@ class test_otptoken_import(object): 'ipatokenserial': u'987654321', 'ipatokenotpdigits': 8, 'type': u'hotp'})] + +if sys.version_info.major == 2: +del t finally: nss.nss_shutdown() @@ -102,7 +109,7 @@ class test_otptoken_import(object): 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 +120,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 @@ -125,7 +132,10 @@ class test_otptoken_import(object): try: doc = PSKCDocument(os.path.join(basename, pskc-mini.xml)) [(t.id, t.options) for t in doc.getKeyPackages()] -except ValidationError: # Unsupported token type. + +if sys.version_info.major == 2: +del t +except ValidationError: # Unsupported token type. pass else: assert False @@ -152,5 +162,8 @@ class test_otptoken_import(object): 'ipatokenotpdigits': 8, 'type': u'hotp', })] + +if sys.version_info.major == 2: +del t finally: nss.nss_shutdown() -- 2.5.0 -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA:
Re: [Freeipa-devel] [patch 0010] Python list comprehension leak breaking the test execution
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
