Skip to content

bpo-37322: Fix ResourceWarning in test_ssl #21393

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

Closed
wants to merge 1 commit into from
Closed

bpo-37322: Fix ResourceWarning in test_ssl #21393

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions Lib/test/test_ssl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

cm.thread.sslconn.close()

def test_pha_optional(self):
if support.verbose:
sys.stdout.write("\n")
Expand Down