-
-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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.
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
bpo-33099: Fix test_poplib hangs after error #6428
Conversation
This fixes resource leaks in the test and reveals real errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please convert try / bare except to try/finally and remove the print_exc line. The same fix should be applied to other tests that use asyncore (ftplib, smtplib, smtpd?)
Lib/test/test_poplib.py
Outdated
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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
.
Lib/test/test_poplib.py
Outdated
self.client = poplib.POP3_SSL(self.server.host, self.server.port) | ||
try: | ||
self.client = poplib.POP3_SSL(self.server.host, self.server.port) | ||
except: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try/finally
Lib/test/test_poplib.py
Outdated
try: | ||
self.client = poplib.POP3(self.server.host, self.server.port, timeout=3) | ||
self.client.stls() | ||
except: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try/finally
Lib/test/test_poplib.py
Outdated
try: | ||
self.client = poplib.POP3(self.server.host, self.server.port, timeout=3) | ||
except: | ||
traceback.print_exc() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you add print_exc?
When you're done making the requested changes, leave the comment: |
Lib/test/test_poplib.py
Outdated
try: | ||
self.client = poplib.POP3(self.server.host, self.server.port, timeout=3) | ||
except: | ||
self.server.stop() |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
"I have made the requested changes; please review again." @tiran I added same cleanup to test_os and test_ftplib. |
Thanks for making the requested changes! @tiran: please review the changes made to this pull request. |
Same fix is commited via #6865. |
This fixes resource leaks in the test and reveals real errors.
https://bugs.python.org/issue33099