bpo-37322: Fix again test_ssl.test_pha_required_nocert() ResourceWarning#14667
bpo-37322: Fix again test_ssl.test_pha_required_nocert() ResourceWarning#14667vstinner wants to merge 1 commit intopython:masterpython/cpython:masterfrom vstinner:test_pha_required_nocert_reswarn2Copy head branch name to clipboard
Conversation
test_ssl.test_pha_required_nocert(): only close the TLS connection at the server side once the handler completed.
|
It seems like the test fails sometimes on Windows. I don't have time right now to investigate, so I will just revert my change instead, PR #14662. |
There was a problem hiding this comment.
Perhaps instead of explicitly using handler.close(), you could use a with statement to guarantee that the resource is safely closed:
with self.ConnectionHandler(self, newconn, connaddr) as handler:
handler.start()
handler.join()This only requires that the object has an __enter__ and __exit__ method, which ConnectionHandler does.
Also as a minor readability change, I would recommend changing this (lines 2457 & 2458):
sys.stdout.write(' server: new connection from '
+ repr(connaddr) + '\n')to:
sys.stdout.write(f"server: new connection from {conaddr!r}\n")This isn't at all necessary, but I figured since it was right near by it would be a good time it clean it up a little. I find the f string formatting a lot easier to quickly read than the old string concatenation.
|
@aeros167: You are commenting a closed PR :-) I abandoned it since the test was still failing randomly with this change when combined with commit 73ea546. |
|
@vstinner Oops, I started typing it while it was still open, my apologies (started typing the review a bit ago and got distracted). Do you think this would be worth reopening the issue over to test if my suggestion fixes the |
test_ssl.test_pha_required_nocert(): only close the TLS connection at
the server side once the handler completed.
https://bugs.python.org/issue37322