Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bpo-33099: Fix test_poplib hangs after error #6428

Closed
wants to merge 5 commits into from
Closed

bpo-33099: Fix test_poplib hangs after error #6428

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
331844a
bpo-33099: Fix test_poplib hangs after error
methane Apr 9, 2018
844e994
Make client fail faster on server error
methane Apr 9, 2018
ac1f692
Remove duplicated traceback.
methane Apr 9, 2018
f3e857d
Use addCleanup() instead of try/except
methane Apr 9, 2018
ab6463f
Use same cleanup for test_os and test_ftplib
methane Apr 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 30 additions & 11 deletions Lib/test/test_poplib.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import os
import errno
import threading
import traceback

from unittest import TestCase, skipUnless
from test import support as test_support
Expand Down Expand Up @@ -220,18 +221,21 @@ 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
self.active = False
self.join()
asyncore.close_all(ignore_all=True)

def handle_accepted(self, conn, addr):
self.handler_instance = self.handler(conn)
Expand All @@ -254,7 +258,12 @@ 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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be try/finally.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When setUp() succeeded, tearDown() do it.
So this should be except:, not finally:.

self.server.stop()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can self.server.stop() fail? Is it worth to rewrite the code as:

try:
    self.server.stop()
finally:
    self.server = None
    raise

or

server = self.server
self.server = None
server.stop()
raise

?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we shouldn't complicate test code for non-real errors.
It's not late to fix it after the possibility become real.

self.server = None
raise

def tearDown(self):
self.client.close()
Expand Down Expand Up @@ -403,7 +412,12 @@ 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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try/finally

self.server.stop()
self.server = None
raise

def test__all__(self):
self.assertIn('POP3_SSL', poplib.__all__)
Expand Down Expand Up @@ -446,8 +460,13 @@ 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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try/finally

self.server.stop()
self.server = None
raise

def tearDown(self):
if self.client.file is not None and self.client.sock is not None:
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2018-04-09-19-02-34.bpo-33099.Bgn7Kc.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix test_poplib hangs when an error occured in ``setUp()`` method or
``DummyPOP3Server.run()``.