bpo-33099: Fix test_poplib hangs after error by methane · Pull Request #6428

bpo-33099: Fix test_poplib hangs after error by methane · Pull Request #6428 · python/cpython

This fixes resource leaks in the test and reveals real errors.

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?)

		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:

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.

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.client = poplib.POP3_SSL(self.server.host, self.server.port)
		try:
		self.client = poplib.POP3_SSL(self.server.host, self.server.port)
		except:

Choose a reason for hiding this comment

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

try/finally

		try:
		self.client = poplib.POP3(self.server.host, self.server.port, timeout=3)
		self.client.stls()
		except:

Choose a reason for hiding this comment

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

try/finally

		try:
		self.client = poplib.POP3(self.server.host, self.server.port, timeout=3)
		except:
		traceback.print_exc()

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: I have made the requested changes; please review again.

		try:
		self.client = poplib.POP3(self.server.host, self.server.port, timeout=3)
		except:
		self.server.stop()

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

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

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.
test_smtplib and test_smtpd has different architecture. They don't use server threads. So they must not hang for dangling threads.

Thanks for making the requested changes!

@tiran: please review the changes made to this pull request.

Same fix is commited via #6865.

jayyyin mannequin mentioned this pull request

May 21, 2022