bpo-37322: Fix ResourceWarning in test_ssl #21393
Conversation
This warning was caused by a socket being left unclosed on the threaded
server in the event of an exception. See issue37322
This exception was observer on my machine running Debian10, output on
master:
$ ./python -m test test_ssl -m test_pha_required_nocert
0:00:00 load avg: 0.08 Run tests sequentially
0:00:00 load avg: 0.08 [1/1] test_ssl
/home/mjh/src/cpython/Lib/test/support/threading_helper.py:209: ResourceWarning: unclosed <ssl.SSLSocket fd=5, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0, laddr=('127.0.0.1', 35919), raddr=('127.0.0.1', 54592)>
del self.thread
ResourceWarning: Enable tracemalloc to get the object allocation traceback
== Tests result: SUCCESS ==
1 test OK.
Total duration: 215 ms
Tests result: SUCCESS
Output with this patch:
$ ./python -m test test_ssl -m test_pha_required_nocert
0:00:00 load avg: 0.24 Run tests sequentially
0:00:00 load avg: 0.24 [1/1] test_ssl
== Tests result: SUCCESS ==
1 test OK.
Total duration: 214 ms
Tests result: SUCCESS
|
Thanks for the review @nanjekyejoannah. Is there anything I can do to help progress this? |
There was a problem hiding this comment.
Let me tag @vstinner. If there is no response I will merge it on monday. I just want to see if anyone who is relevant has an opinion.
| @@ -4447,6 +4447,11 @@ def test_pha_required_nocert(self): | |||
| 'tlsv13 alert certificate required'): | |||
| s.recv(1024) | |||
|
|
|||
| # If there was an exception, manually close the socket to avoid a | |||
| # ResourceWarning | |||
| if cm.exc_type == ssl.SSLError: | |||
There was a problem hiding this comment.
I don't know this code so I don't know if the fix is correct or not. Why not always closing all connections? (why only on error)
There was a problem hiding this comment.
Thanks for jumping in, and good question, I'd like to know why I didn't put that info in my commit 🤦
I'm not sure what I was thinking at the time, but I'm no longer certain my change is the best handling of this situation.
Incoming brain dump
Why not always closing all connections?
In the case of no error this would be closed by ThreadedEchoServer.ConnectionHandler.close. However, the test above causes ThreadedEchoServer.ConnectionHandler.read to raise an exception in ThreadedEchoServer.ConnectionHandler.run which is then caught at line 2489 of this file:
except ssl.SSLError as err:
# On Windows sometimes test_pha_required_nocert receives the
# PEER_DID_NOT_RETURN_A_CERTIFICATE exception
# before the 'tlsv13 alert certificate required' exception.
# If the server is stopped when PEER_DID_NOT_RETURN_A_CERTIFICATE
# is received test_pha_required_nocert fails with ConnectionResetError
# because the underlying socket is closed
if 'PEER_DID_NOT_RETURN_A_CERTIFICATE' == err.reason:
if self.server.chatty and support.verbose:
sys.stdout.write(err.args[1])
# test_pha_required_nocert is expecting this exception
raise ssl.SSLError('tlsv13 alert certificate required')
Though it looks like you're already had some experience with this resource warning in that location:
I will note that I'm seeing that condition while not running on Windows (nor do I have one on hand at the moment), so not sure if that comment needs updating.
End brain dump
Something else that probably should've been in the coommit is some more context wtih tracemalloc:
$ PYTHONTRACEMALLOC=25 ./python -m test test_ssl -m test_pha_required_nocert
0:00:00 load avg: 0.63 Run tests sequentially
0:00:00 load avg: 0.63 [1/1] test_ssl
/home/mjh/src/cpython/Lib/test/support/threading_helper.py:209: ResourceWarning: unclosed <ssl.SSLSocket fd=5, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0, laddr=('127.0.0.1', 58049), raddr=('127.0.0.1', 45368)>
del self.thread
Object allocated at (most recent call last):
File "/home/mjh/src/cpython/Lib/threading.py", lineno 908
self._bootstrap_inner()
File "/home/mjh/src/cpython/Lib/threading.py", lineno 950
self.run()
File "/home/mjh/src/cpython/Lib/test/test_ssl.py", lineno 2413
if not self.wrap_conn():
File "/home/mjh/src/cpython/Lib/test/test_ssl.py", lineno 2334
self.sslconn = self.server.context.wrap_socket(
File "/home/mjh/src/cpython/Lib/ssl.py", lineno 500
return self.sslsocket_class._create(
File "/home/mjh/src/cpython/Lib/ssl.py", lineno 1003
self = cls.__new__(cls, **kwargs)
== Tests result: SUCCESS ==
1 test OK.
Total duration: 387 ms
Tests result: SUCCESS
|
Apologies having neglected this. I think I might close this, since without a Windows machine to play around on I'm not confident any change I implement won't potentially introduce intermittent errors there. |
https://bugs.python.org/issue37322
This is a minor change to a test so I didn't think it warranted a NEWS entry, but happy to add one if I am mistaken.
This warning was caused by a socket being left unclosed on the threaded
server in the event of an exception. See issue37322
This exception was observed on my machine running Debian10, output on
master:
Output with this patch:
https://bugs.python.org/issue37322