From 331844afe124d66e790b5ea7843bccac12288390 Mon Sep 17 00:00:00 2001 From: INADA Naoki Date: Mon, 9 Apr 2018 19:02:45 +0900 Subject: [PATCH 1/5] bpo-33099: Fix test_poplib hangs after error This fixes resource leaks in the test and reveals real errors. --- Lib/test/test_poplib.py | 29 +++++++++++++++---- .../2018-04-09-19-02-34.bpo-33099.Bgn7Kc.rst | 2 ++ 2 files changed, 26 insertions(+), 5 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2018-04-09-19-02-34.bpo-33099.Bgn7Kc.rst diff --git a/Lib/test/test_poplib.py b/Lib/test/test_poplib.py index bf568bd77bffaf..2f02b4bcfbc8ff 100644 --- a/Lib/test/test_poplib.py +++ b/Lib/test/test_poplib.py @@ -10,6 +10,7 @@ import os import errno import threading +import traceback from unittest import TestCase, skipUnless from test import support as test_support @@ -226,12 +227,12 @@ def run(self): self.active_lock.acquire() asyncore.loop(timeout=0.1, count=1) self.active_lock.release() - asyncore.close_all(ignore_all=True) def stop(self): assert self.active self.active = False self.join() + asyncore.close_all(ignore_all=True) def handle_accepted(self, conn, addr): self.handler_instance = self.handler(conn) @@ -254,7 +255,13 @@ def assertOK(self, resp): def setUp(self): self.server = DummyPOP3Server((HOST, PORT)) self.server.start() - self.client = poplib.POP3(self.server.host, self.server.port, timeout=3) + try: + self.client = poplib.POP3(self.server.host, self.server.port, timeout=3) + except: + traceback.print_exc() + self.server.stop() + self.server = None + raise def tearDown(self): self.client.close() @@ -403,7 +410,13 @@ def setUp(self): self.server = DummyPOP3Server((HOST, PORT)) self.server.handler = DummyPOP3_SSLHandler self.server.start() - self.client = poplib.POP3_SSL(self.server.host, self.server.port) + try: + self.client = poplib.POP3_SSL(self.server.host, self.server.port) + except: + traceback.print_exc() + self.server.stop() + self.server = None + raise def test__all__(self): self.assertIn('POP3_SSL', poplib.__all__) @@ -446,8 +459,14 @@ class TestPOP3_TLSClass(TestPOP3Class): def setUp(self): self.server = DummyPOP3Server((HOST, PORT)) self.server.start() - self.client = poplib.POP3(self.server.host, self.server.port, timeout=3) - self.client.stls() + try: + self.client = poplib.POP3(self.server.host, self.server.port, timeout=3) + self.client.stls() + except: + traceback.print_exc() + self.server.stop() + self.server = None + raise def tearDown(self): if self.client.file is not None and self.client.sock is not None: diff --git a/Misc/NEWS.d/next/Library/2018-04-09-19-02-34.bpo-33099.Bgn7Kc.rst b/Misc/NEWS.d/next/Library/2018-04-09-19-02-34.bpo-33099.Bgn7Kc.rst new file mode 100644 index 00000000000000..3748c1fa6699c6 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2018-04-09-19-02-34.bpo-33099.Bgn7Kc.rst @@ -0,0 +1,2 @@ +Fix test_poplib hangs when an error occured in ``setUp()`` method or +``DummyPOP3Server.run()``. From 844e994c0f5c375893edc101ffe1cbe24db99b03 Mon Sep 17 00:00:00 2001 From: INADA Naoki Date: Mon, 9 Apr 2018 19:16:21 +0900 Subject: [PATCH 2/5] Make client fail faster on server error --- Lib/test/test_poplib.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/Lib/test/test_poplib.py b/Lib/test/test_poplib.py index 2f02b4bcfbc8ff..46c0a79970d711 100644 --- a/Lib/test/test_poplib.py +++ b/Lib/test/test_poplib.py @@ -221,12 +221,15 @@ def start(self): self.__flag.wait() def run(self): - self.active = True - self.__flag.set() - while self.active and asyncore.socket_map: - self.active_lock.acquire() - asyncore.loop(timeout=0.1, count=1) - self.active_lock.release() + try: + self.active = True + self.__flag.set() + while self.active and asyncore.socket_map: + self.active_lock.acquire() + asyncore.loop(timeout=0.1, count=1) + self.active_lock.release() + finally: + self.close() def stop(self): assert self.active From ac1f6929c00092b6b77471789a2e93bdf4b5af78 Mon Sep 17 00:00:00 2001 From: INADA Naoki Date: Mon, 9 Apr 2018 19:42:51 +0900 Subject: [PATCH 3/5] Remove duplicated traceback. --- Lib/test/test_poplib.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/Lib/test/test_poplib.py b/Lib/test/test_poplib.py index 46c0a79970d711..9c7b22b7bad75e 100644 --- a/Lib/test/test_poplib.py +++ b/Lib/test/test_poplib.py @@ -261,7 +261,6 @@ def setUp(self): try: self.client = poplib.POP3(self.server.host, self.server.port, timeout=3) except: - traceback.print_exc() self.server.stop() self.server = None raise @@ -416,7 +415,6 @@ def setUp(self): try: self.client = poplib.POP3_SSL(self.server.host, self.server.port) except: - traceback.print_exc() self.server.stop() self.server = None raise @@ -466,7 +464,6 @@ def setUp(self): self.client = poplib.POP3(self.server.host, self.server.port, timeout=3) self.client.stls() except: - traceback.print_exc() self.server.stop() self.server = None raise From f3e857d4c4a468e58c7645a61f94a5ed4643b297 Mon Sep 17 00:00:00 2001 From: INADA Naoki Date: Mon, 9 Apr 2018 20:21:21 +0900 Subject: [PATCH 4/5] Use addCleanup() instead of try/except --- Lib/test/test_poplib.py | 45 +++++++++++++++-------------------------- 1 file changed, 16 insertions(+), 29 deletions(-) diff --git a/Lib/test/test_poplib.py b/Lib/test/test_poplib.py index 9c7b22b7bad75e..6abdc93879395a 100644 --- a/Lib/test/test_poplib.py +++ b/Lib/test/test_poplib.py @@ -255,21 +255,24 @@ class TestPOP3Class(TestCase): def assertOK(self, resp): self.assertTrue(resp.startswith(b"+OK")) - def setUp(self): + def setup_server(self, handler_class=None): self.server = DummyPOP3Server((HOST, PORT)) + if handler_class is not None: + self.server.handler = handler_class self.server.start() - try: - self.client = poplib.POP3(self.server.host, self.server.port, timeout=3) - except: + + @self.addCleanup + def close_server(): self.server.stop() + # Explicitly clear the attribute to prevent dangling thread self.server = None - raise + + def setUp(self): + self.setup_server() + self.client = poplib.POP3(self.server.host, self.server.port, timeout=3) def tearDown(self): self.client.close() - self.server.stop() - # Explicitly clear the attribute to prevent dangling thread - self.server = None def test_getwelcome(self): self.assertEqual(self.client.getwelcome(), @@ -409,15 +412,8 @@ class TestPOP3_SSLClass(TestPOP3Class): # repeat previous tests by using poplib.POP3_SSL def setUp(self): - self.server = DummyPOP3Server((HOST, PORT)) - self.server.handler = DummyPOP3_SSLHandler - self.server.start() - try: - self.client = poplib.POP3_SSL(self.server.host, self.server.port) - except: - self.server.stop() - self.server = None - raise + self.setup_server(DummyPOP3_SSLHandler) + self.client = poplib.POP3_SSL(self.server.host, self.server.port) def test__all__(self): self.assertIn('POP3_SSL', poplib.__all__) @@ -458,15 +454,9 @@ class TestPOP3_TLSClass(TestPOP3Class): # repeat previous tests by using poplib.POP3.stls() def setUp(self): - self.server = DummyPOP3Server((HOST, PORT)) - self.server.start() - try: - self.client = poplib.POP3(self.server.host, self.server.port, timeout=3) - self.client.stls() - except: - self.server.stop() - self.server = None - raise + self.setup_server() + self.client = poplib.POP3(self.server.host, self.server.port, timeout=3) + self.client.stls() def tearDown(self): if self.client.file is not None and self.client.sock is not None: @@ -477,9 +467,6 @@ def tearDown(self): # response will be treated as response to QUIT and raise # this exception self.client.close() - self.server.stop() - # Explicitly clear the attribute to prevent dangling thread - self.server = None def test_stls(self): self.assertRaises(poplib.error_proto, self.client.stls) From ab6463fd7200c742c1a1ce4068a0292169e23112 Mon Sep 17 00:00:00 2001 From: INADA Naoki Date: Mon, 9 Apr 2018 21:30:38 +0900 Subject: [PATCH 5/5] Use same cleanup for test_os and test_ftplib --- Lib/test/test_ftplib.py | 52 ++++++++++++++++++++--------------------- Lib/test/test_os.py | 34 +++++++++++++++++---------- 2 files changed, 47 insertions(+), 39 deletions(-) diff --git a/Lib/test/test_ftplib.py b/Lib/test/test_ftplib.py index 1a8e2f91d386df..462d5d6d4c4428 100644 --- a/Lib/test/test_ftplib.py +++ b/Lib/test/test_ftplib.py @@ -272,13 +272,15 @@ def start(self): self.__flag.wait() def run(self): - self.active = True - self.__flag.set() - while self.active and asyncore.socket_map: - self.active_lock.acquire() - asyncore.loop(timeout=0.1, count=1) - self.active_lock.release() - asyncore.close_all(ignore_all=True) + try: + self.active = True + self.__flag.set() + while self.active and asyncore.socket_map: + self.active_lock.acquire() + asyncore.loop(timeout=0.1, count=1) + self.active_lock.release() + finally: + self.close() def stop(self): assert self.active @@ -464,20 +466,27 @@ class DummyTLS_FTPServer(DummyFTPServer): handler = DummyTLS_FTPHandler +def setup_server(self, server): + self.server = server + self.server.start() + + @self.addCleanup + def close_server(): + self.server.stop() + # Explicitly clear the attribute to prevent dangling thread + self.server = None + asyncore.close_all(ignore_all=True) + + class TestFTPClass(TestCase): def setUp(self): - self.server = DummyFTPServer((HOST, 0)) - self.server.start() + setup_server(self, DummyFTPServer((HOST, 0))) self.client = ftplib.FTP(timeout=TIMEOUT) self.client.connect(self.server.host, self.server.port) def tearDown(self): self.client.close() - self.server.stop() - # Explicitly clear the attribute to prevent dangling thread - self.server = None - asyncore.close_all(ignore_all=True) def check_data(self, received, expected): self.assertEqual(len(received), len(expected)) @@ -799,17 +808,12 @@ def test_storlines_too_long(self): class TestIPv6Environment(TestCase): def setUp(self): - self.server = DummyFTPServer((HOSTv6, 0), af=socket.AF_INET6) - self.server.start() + setup_server(self, DummyFTPServer((HOSTv6, 0), af=socket.AF_INET6)) self.client = ftplib.FTP(timeout=TIMEOUT) self.client.connect(self.server.host, self.server.port) def tearDown(self): self.client.close() - self.server.stop() - # Explicitly clear the attribute to prevent dangling thread - self.server = None - asyncore.close_all(ignore_all=True) def test_af(self): self.assertEqual(self.client.af, socket.AF_INET6) @@ -846,8 +850,7 @@ class TestTLS_FTPClassMixin(TestFTPClass): """ def setUp(self): - self.server = DummyTLS_FTPServer((HOST, 0)) - self.server.start() + setup_server(self, DummyTLS_FTPServer((HOST, 0))) self.client = ftplib.FTP_TLS(timeout=TIMEOUT) self.client.connect(self.server.host, self.server.port) # enable TLS @@ -860,17 +863,12 @@ class TestTLS_FTPClass(TestCase): """Specific TLS_FTP class tests.""" def setUp(self): - self.server = DummyTLS_FTPServer((HOST, 0)) - self.server.start() + setup_server(self, DummyTLS_FTPServer((HOST, 0))) self.client = ftplib.FTP_TLS(timeout=TIMEOUT) self.client.connect(self.server.host, self.server.port) def tearDown(self): self.client.close() - self.server.stop() - # Explicitly clear the attribute to prevent dangling thread - self.server = None - asyncore.close_all(ignore_all=True) def test_control_connection(self): self.assertNotIsInstance(self.client.sock, ssl.SSLSocket) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index e509188243f697..73c1f4f0add1e0 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -2586,13 +2586,15 @@ def wait(self): # --- internals def run(self): - self._active = True - self.__flag.set() - while self._active and asyncore.socket_map: - self._active_lock.acquire() - asyncore.loop(timeout=0.001, count=1) - self._active_lock.release() - asyncore.close_all() + try: + self._active = True + self.__flag.set() + while self._active and asyncore.socket_map: + self._active_lock.acquire() + asyncore.loop(timeout=0.001, count=1) + self._active_lock.release() + finally: + self.close() def handle_accept(self): conn, addr = self.accept() @@ -2629,9 +2631,20 @@ def tearDownClass(cls): support.threading_cleanup(*cls.key) support.unlink(support.TESTFN) - def setUp(self): - self.server = SendfileTestServer((support.HOST, 0)) + def setup_server(self, server): + self.server = server self.server.start() + + @self.addCleanup + def close_server(): + if self.server.running: + self.server.stop() + # Explicitly clear the attribute to prevent dangling thread + self.server = None + asyncore.close_all(ignore_all=True) + + def setUp(self): + self.setup_server(SendfileTestServer((support.HOST, 0))) self.client = socket.socket() self.client.connect((self.server.host, self.server.port)) self.client.settimeout(1) @@ -2644,9 +2657,6 @@ def setUp(self): def tearDown(self): self.file.close() self.client.close() - if self.server.running: - self.server.stop() - self.server = None def sendfile_wrapper(self, sock, file, offset, nbytes, headers=[], trailers=[]): """A higher level wrapper representing how an application is